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

[Dave Jiang <dave.jiang@intel.com>]

On 8/2/23 07:58, Fenghua Yu wrote:
> 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 <fenghua.yu@intel.com>
>>> Reviewed-by: Ramesh Thomas <ramesh.thomas@intel.com>
>>> ---
>>>   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?

Ok


> 
> 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