Re: [Accel-config] [PATCH 2/7] accel-config: Add config-user-default command

Hi, Dave,

On 8/1/23 17:14, Dave Jiang wrote:
> 
> 
> On 8/1/23 14:52, Fenghua Yu wrote:
>> Although current accel-config provides thorough ways to configure
>> IDXD devices and WQs, sometimes user needs an easier way to configure
>> and enable them without many knowledges of IDXD.
>>
>> A new command "config-user-default" is added to configure and enable
>> all available devices and WQs for user usage in the following default
>> configurations:
>>
>> 1. Fixed configurations:
>>     "mode":"shared",
>>     "group_id":0,
>>     "priority":10,
>>     "block_on_fault":1,
>>     "name":"user_default_wq",
>>     "ats_disable":0,
>>     "prs_disable":1
>>     "type": "user"
>>     "driver_name":"user",
>>
>> 2. Calculated configurations:
>>     "size": max WQ size / max WQs
>>     "threshold": WQ size
>>
>> 3. Default configurations that have been set by IDXD driver:
>>     "max_batch_size"
>>     "max_transfer_size"
>>     "op_config"
>>
>> Signed-off-by: Fenghua Yu <[email protected]>
>> Reviewed-by: Ramesh Thomas <[email protected]>
>> ---
>>   accfg/accel-config.c    |   1 +
>>   accfg/config.c          | 268 ++++++++++++++++++++++++++++++++++++++++
>>   accfg/libaccel_config.h |   1 +
>>   builtin.h               |   1 +
>>   4 files changed, 271 insertions(+)
>>
>> diff --git a/accfg/accel-config.c b/accfg/accel-config.c
>> index a897608..cf3882e 100644
>> --- a/accfg/accel-config.c
>> +++ b/accfg/accel-config.c
>> @@ -67,6 +67,7 @@ static struct cmd_struct commands[] = {
>>       {"config-group", cmd_config_group},
>>       {"config-wq", cmd_config_wq},
>>       {"config-engine", cmd_config_engine},
>> +    {"config-user-default", cmd_config_default},
>>   #ifdef ENABLE_TEST
>>       {"test", cmd_test},
>>   #endif
>> diff --git a/accfg/config.c b/accfg/config.c
>> index 2631fd9..f3fa83f 100644
>> --- a/accfg/config.c
>> +++ b/accfg/config.c
>> @@ -119,6 +119,87 @@ static bool is_wq_threshold_writable(struct 
>> accfg_wq *wq, int val);
>>   static bool is_wq_prs_disable_writable(struct accfg_wq *wq, int val);
>>   static bool is_wq_ats_disable_writable(struct accfg_wq *wq, int val);
>> +static int get_wq_size(struct accfg_device *dev)
>> +{
>> +    int max_wq_size, max_wqs;
>> +
>> +    max_wq_size = accfg_device_get_max_work_queues_size(dev);
>> +    max_wqs = accfg_device_get_max_work_queues(dev);
>> +
>> +    return max_wq_size / max_wqs;
>> +}
>> +
>> +static int config_default_wq_set_prs_disable(struct accfg_wq *wq, int 
>> val)
>> +{
>> +    if (!is_wq_prs_disable_writable(wq, val))
>> +        return -EPERM;
>> +
>> +    return accfg_wq_set_prs_disable(wq, val);
>> +}
>> +
>> +static int config_default_wq_set_ats_disable(struct accfg_wq *wq, int 
>> val)
>> +{
>> +    if (!is_wq_ats_disable_writable(wq, val))
>> +        return -EPERM;
>> +
>> +    return accfg_wq_set_ats_disable(wq, val);
>> +}
>> +
>> +static int config_default_wq_set_threshold(struct accfg_wq *wq, int val)
>> +{
>> +    if (!is_wq_threshold_writable(wq, val))
>> +        return -EPERM;

threshold only be writable on shared group.

>> +
>> +    return accfg_wq_set_threshold(wq, val);
>> +}
>> +
>> +static struct conf_def_wq_param {
>> +    struct wq_parameters param;
>> +    bool configured;
>> +} conf_def_wq_param[ACCFG_DEVICE_MAX];
>> +
>> +/* Return WQ parameter for dev type. */
>> +static struct wq_parameters *get_conf_def_wq_param(enum 
>> accfg_device_type type)
>> +{
>> +    if (type == ACCFG_DEVICE_DSA)
>> +        return &conf_def_wq_param[ACCFG_DEVICE_DSA].param;
>> +    else if (type == ACCFG_DEVICE_IAX)
>> +        return &conf_def_wq_param[ACCFG_DEVICE_IAX].param;
>> +
>> +    return NULL;
>> +}
>> +
>> +/* Check if dev is configured. */
>> +static bool conf_def_dev_configured(struct accfg_device *dev)
>> +{
>> +    if (accfg_device_get_type(dev) == ACCFG_DEVICE_DSA)
>> +        return conf_def_wq_param[ACCFG_DEVICE_DSA].configured;
>> +    else if (accfg_device_get_type(dev) == ACCFG_DEVICE_IAX)
>> +        return conf_def_wq_param[ACCFG_DEVICE_IAX].configured;
>> +
>> +    return false;
>> +}
>> +
>> +/* Set WQ parameters based on device cap: size and threshold. */
>> +static int config_default_wq_set_on_dev(struct accfg_device *dev)
>> +{
>> +    enum accfg_device_type dev_type;
>> +    struct wq_parameters *p;
>> +
>> +    dev_type = accfg_device_get_type(dev);
>> +    p = get_conf_def_wq_param(dev_type);
>> +    if (!p)
>> +        return -EINVAL;
>> +
>> +    p->wq_size = get_wq_size(dev);
>> +    if (p->wq_size <= 0)
>> +        return -ENOSPC;
>> +
>> +    p->threshold = p->wq_size;
> 
> Should check if wq is shared before setting threshold. threshold has no 
> meaning for dwq.

This is just writing an internal parameter. No need to check mode here, 
right?

The mode check happens during writing threshold via sysfs. Please see 
this functions:

 >> +static int config_default_wq_set_threshold(struct accfg_wq *wq, int 
val)
 >> +{
 >> +    if (!is_wq_threshold_writable(wq, val))
 >> +        return -EPERM;

Thanks.

-Fenghua