[PATCH] dma/idxd: add generic option for queue config

[PATCH] dma/idxd: add generic option for queue config

[Kevin Laatz]

The device config script currently uses some defaults to configure
devices in a generic way.

With the addition of this option, users have more control over how
queues are configured.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/dpdk_idxd_cfg.py | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
index 3f5d5ee752..9ac724e7a8 100755
--- a/drivers/dma/idxd/dpdk_idxd_cfg.py
+++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
@@ -62,9 +62,25 @@ def get_dsa_id(pci):
                 return int(dir[3:])
     sys.exit(f"Could not get device ID for device {pci}")
 
-
-def configure_dsa(dsa_id, queues, prefix):
+def parse_wq_opts(dsa_id, q, wq_opts):
+    "Parse the additional user-specified queue configuration"
+    wq_dir = SysfsDir(f'/sys/bus/dsa/devices/dsa{dsa_id}/wq{dsa_id}.{q}')
+    for wq_opt in wq_opts:
+        try:
+            opt, val = wq_opt.split("=")
+        except ValueError:
+            sys.exit("Invalid format, use format 'option=value'")
+        if not os.path.exists(os.path.join(wq_dir.path, f'{opt}')):
+            sys.exit(f"Invalid sysfs node '{opt}', path does not exist")
+        wq_dir.write_values({opt: val})
+
+
+def configure_dsa(dsa_id, args):
     "Configure the DSA instance with appropriate number of queues"
+    queues = args.q
+    prefix = args.prefix
+    wq_opts = args.wq_option
+
     dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
 
     max_groups = dsa_dir.read_int("max_groups")
@@ -92,6 +108,11 @@ def configure_dsa(dsa_id, queues, prefix):
                              "max_batch_size": 1024,
                              "size": int(max_work_queues_size / nb_queues)})
 
+    # parse additional user-spcified queue configuration
+    if wq_opts:
+        for q in range(nb_queues):
+            parse_wq_opts(dsa_id, q, wq_opts)
+
     # enable device and then queues
     idxd_dir = SysfsDir(get_drv_dir("idxd"))
     idxd_dir.write_values({"bind": f"dsa{dsa_id}"})
@@ -112,6 +133,8 @@ def main(args):
     arg_p.add_argument('--name-prefix', metavar='prefix', dest='prefix',
                        default="dpdk",
                        help="Prefix for workqueue name to mark for DPDK use [default: 'dpdk']")
+    arg_p.add_argument('--wq-option', action='append',
+                       help="Provide additional config option for queues (format 'x=y')")
     arg_p.add_argument('--reset', action='store_true',
                        help="Reset DSA device and its queues")
     parsed_args = arg_p.parse_args(args[1:])
@@ -121,7 +144,7 @@ def main(args):
     if parsed_args.reset:
         reset_device(dsa_id)
     else:
-        configure_dsa(dsa_id, parsed_args.q, parsed_args.prefix)
+        configure_dsa(dsa_id, parsed_args)
 
 
 if __name__ == "__main__":
-- 
2.31.1

Re: [PATCH] dma/idxd: add generic option for queue config

[Bruce Richardson]

On Wed, Mar 30, 2022 at 04:07:00PM +0100, Kevin Laatz wrote:
> The device config script currently uses some defaults to configure
> devices in a generic way.
> 
> With the addition of this option, users have more control over how
> queues are configured.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/dpdk_idxd_cfg.py | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
> index 3f5d5ee752..9ac724e7a8 100755
> --- a/drivers/dma/idxd/dpdk_idxd_cfg.py
> +++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
> @@ -62,9 +62,25 @@ def get_dsa_id(pci):
>                  return int(dir[3:])
>      sys.exit(f"Could not get device ID for device {pci}")
>  
> -
> -def configure_dsa(dsa_id, queues, prefix):
> +def parse_wq_opts(dsa_id, q, wq_opts):
> +    "Parse the additional user-specified queue configuration"
> +    wq_dir = SysfsDir(f'/sys/bus/dsa/devices/dsa{dsa_id}/wq{dsa_id}.{q}')
> +    for wq_opt in wq_opts:
> +        try:
> +            opt, val = wq_opt.split("=")
> +        except ValueError:
> +            sys.exit("Invalid format, use format 'option=value'")
> +        if not os.path.exists(os.path.join(wq_dir.path, f'{opt}')):
> +            sys.exit(f"Invalid sysfs node '{opt}', path does not exist")
> +        wq_dir.write_values({opt: val})
> +
> +
> +def configure_dsa(dsa_id, args):
>      "Configure the DSA instance with appropriate number of queues"
> +    queues = args.q
> +    prefix = args.prefix
> +    wq_opts = args.wq_option
> +
>      dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
>  
>      max_groups = dsa_dir.read_int("max_groups")
> @@ -92,6 +108,11 @@ def configure_dsa(dsa_id, queues, prefix):
>                               "max_batch_size": 1024,
>                               "size": int(max_work_queues_size / nb_queues)})
>  
> +    # parse additional user-spcified queue configuration
> +    if wq_opts:
> +        for q in range(nb_queues):
> +            parse_wq_opts(dsa_id, q, wq_opts)
> +

I think this may be better to have the parse function only parse the
options and split them. If that is done before the actual queue
configuration function is called, then the additional options could be
passed in there, and merged with the existing config settings. This avoids
duplicating things and doing two sets of configs.

/Bruce

Re: [PATCH] dma/idxd: add generic option for queue config

[Kevin Laatz]

On 31/03/2022 15:57, Bruce Richardson wrote:
> On Wed, Mar 30, 2022 at 04:07:00PM +0100, Kevin Laatz wrote:
>> The device config script currently uses some defaults to configure
>> devices in a generic way.
>>
>> With the addition of this option, users have more control over how
>> queues are configured.
>>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
>>   drivers/dma/idxd/dpdk_idxd_cfg.py | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
>> index 3f5d5ee752..9ac724e7a8 100755
>> --- a/drivers/dma/idxd/dpdk_idxd_cfg.py
>> +++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
>> @@ -62,9 +62,25 @@ def get_dsa_id(pci):
>>                   return int(dir[3:])
>>       sys.exit(f"Could not get device ID for device {pci}")
>>   
>> -
>> -def configure_dsa(dsa_id, queues, prefix):
>> +def parse_wq_opts(dsa_id, q, wq_opts):
>> +    "Parse the additional user-specified queue configuration"
>> +    wq_dir = SysfsDir(f'/sys/bus/dsa/devices/dsa{dsa_id}/wq{dsa_id}.{q}')
>> +    for wq_opt in wq_opts:
>> +        try:
>> +            opt, val = wq_opt.split("=")
>> +        except ValueError:
>> +            sys.exit("Invalid format, use format 'option=value'")
>> +        if not os.path.exists(os.path.join(wq_dir.path, f'{opt}')):
>> +            sys.exit(f"Invalid sysfs node '{opt}', path does not exist")
>> +        wq_dir.write_values({opt: val})
>> +
>> +
>> +def configure_dsa(dsa_id, args):
>>       "Configure the DSA instance with appropriate number of queues"
>> +    queues = args.q
>> +    prefix = args.prefix
>> +    wq_opts = args.wq_option
>> +
>>       dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
>>   
>>       max_groups = dsa_dir.read_int("max_groups")
>> @@ -92,6 +108,11 @@ def configure_dsa(dsa_id, queues, prefix):
>>                                "max_batch_size": 1024,
>>                                "size": int(max_work_queues_size / nb_queues)})
>>   
>> +    # parse additional user-spcified queue configuration
>> +    if wq_opts:
>> +        for q in range(nb_queues):
>> +            parse_wq_opts(dsa_id, q, wq_opts)
>> +
> I think this may be better to have the parse function only parse the
> options and split them. If that is done before the actual queue
> configuration function is called, then the additional options could be
> passed in there, and merged with the existing config settings. This avoids
> duplicating things and doing two sets of configs.

Thanks for the suggestion, Bruce. I'll look into it and send a v2.

/Kevin

[PATCH v2] dma/idxd: add generic option for queue config

[Kevin Laatz]

The device config script currently uses some defaults to configure
devices in a generic way.

With the addition of this option, users have more control over how
queues are configured.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/dpdk_idxd_cfg.py | 39 ++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
index 3f5d5ee752..704cf775ed 100755
--- a/drivers/dma/idxd/dpdk_idxd_cfg.py
+++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
@@ -63,8 +63,24 @@ def get_dsa_id(pci):
     sys.exit(f"Could not get device ID for device {pci}")
 
 
-def configure_dsa(dsa_id, queues, prefix):
+def parse_wq_opts(wq_opts):
+    "Parse user-specified queue configuration, creating a dict of options"
+    opt_dict = {}
+    for wq_opt in wq_opts:
+        try:
+            opt, val = wq_opt.split("=")
+        except ValueError:
+            sys.exit("Invalid --wq-option format, use format 'option=value'")
+        opt_dict[opt] = val
+    return opt_dict
+
+
+def configure_dsa(dsa_id, args):
     "Configure the DSA instance with appropriate number of queues"
+    queues = args.q
+    prefix = args.prefix
+    wq_opts = args.wq_option
+
     dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
 
     max_groups = dsa_dir.read_int("max_groups")
@@ -83,14 +99,17 @@ def configure_dsa(dsa_id, queues, prefix):
 
     # configure each queue
     for q in range(nb_queues):
+        default_dict = {"group_id": q % nb_groups,
+                        "type": "user",
+                        "mode": "dedicated",
+                        "name": f"{prefix}_wq{dsa_id}.{q}",
+                        "priority": 1,
+                        "max_batch_size": 1024,
+                        "size": int(max_work_queues_size / nb_queues)}
+        config_dict = parse_wq_opts(wq_opts)
+        write_dict = {**default_dict, **config_dict}
         wq_dir = SysfsDir(os.path.join(dsa_dir.path, f"wq{dsa_id}.{q}"))
-        wq_dir.write_values({"group_id": q % nb_groups,
-                             "type": "user",
-                             "mode": "dedicated",
-                             "name": f"{prefix}_wq{dsa_id}.{q}",
-                             "priority": 1,
-                             "max_batch_size": 1024,
-                             "size": int(max_work_queues_size / nb_queues)})
+        wq_dir.write_values(write_dict)
 
     # enable device and then queues
     idxd_dir = SysfsDir(get_drv_dir("idxd"))
@@ -112,6 +131,8 @@ def main(args):
     arg_p.add_argument('--name-prefix', metavar='prefix', dest='prefix',
                        default="dpdk",
                        help="Prefix for workqueue name to mark for DPDK use [default: 'dpdk']")
+    arg_p.add_argument('--wq-option', action='append',
+                       help="Provide additional config option for queues (format 'x=y')")
     arg_p.add_argument('--reset', action='store_true',
                        help="Reset DSA device and its queues")
     parsed_args = arg_p.parse_args(args[1:])
@@ -121,7 +142,7 @@ def main(args):
     if parsed_args.reset:
         reset_device(dsa_id)
     else:
-        configure_dsa(dsa_id, parsed_args.q, parsed_args.prefix)
+        configure_dsa(dsa_id, parsed_args)
 
 
 if __name__ == "__main__":
-- 
2.31.1

Re: [PATCH v2] dma/idxd: add generic option for queue config

[Bruce Richardson]

On Thu, Mar 31, 2022 at 06:21:12PM +0100, Kevin Laatz wrote:
> The device config script currently uses some defaults to configure
> devices in a generic way.
> 
> With the addition of this option, users have more control over how
> queues are configured.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/dpdk_idxd_cfg.py | 39 ++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
> index 3f5d5ee752..704cf775ed 100755
> --- a/drivers/dma/idxd/dpdk_idxd_cfg.py
> +++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
> @@ -63,8 +63,24 @@ def get_dsa_id(pci):
>      sys.exit(f"Could not get device ID for device {pci}")
>  
>  
> -def configure_dsa(dsa_id, queues, prefix):
> +def parse_wq_opts(wq_opts):
> +    "Parse user-specified queue configuration, creating a dict of options"
> +    opt_dict = {}
> +    for wq_opt in wq_opts:
> +        try:
> +            opt, val = wq_opt.split("=")
> +        except ValueError:
> +            sys.exit("Invalid --wq-option format, use format 'option=value'")
> +        opt_dict[opt] = val
> +    return opt_dict
> +

Can I suggest a shorter version of this:

def parse_wq_opts(wq_opts):
    "Parse ... "
    try:
        return {o.split('=')[0]: o.split('=')[1] for o in wq_opts}
    except:
        ...


> +
> +def configure_dsa(dsa_id, args):
>      "Configure the DSA instance with appropriate number of queues"
> +    queues = args.q
> +    prefix = args.prefix
> +    wq_opts = args.wq_option
> +

Do we really save much using all these variables? The "queues" one might be
useful and clearer, but I wonder how much we save by avoiding the "args."
part of the rest? For example, wq_opts is only used once.

>      dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
>  
>      max_groups = dsa_dir.read_int("max_groups")
> @@ -83,14 +99,17 @@ def configure_dsa(dsa_id, queues, prefix):
>  
>      # configure each queue
>      for q in range(nb_queues):
> +        default_dict = {"group_id": q % nb_groups,
> +                        "type": "user",
> +                        "mode": "dedicated",
> +                        "name": f"{prefix}_wq{dsa_id}.{q}",
> +                        "priority": 1,
> +                        "max_batch_size": 1024,
> +                        "size": int(max_work_queues_size / nb_queues)}
> +        config_dict = parse_wq_opts(wq_opts)
> +        write_dict = {**default_dict, **config_dict}

These two lines could be replaced by:
	default_dict.update(parse_wq_opts(...))
though in this case, you probably want to rename default_dict to
write_dict - or maybe even to "qcfg" or something more meaningful.

>          wq_dir = SysfsDir(os.path.join(dsa_dir.path, f"wq{dsa_id}.{q}"))
> -        wq_dir.write_values({"group_id": q % nb_groups,
> -                             "type": "user",
> -                             "mode": "dedicated",
> -                             "name": f"{prefix}_wq{dsa_id}.{q}",
> -                             "priority": 1,
> -                             "max_batch_size": 1024,
> -                             "size": int(max_work_queues_size / nb_queues)})
> +        wq_dir.write_values(write_dict)
>  
>      # enable device and then queues
>      idxd_dir = SysfsDir(get_drv_dir("idxd"))
> @@ -112,6 +131,8 @@ def main(args):
>      arg_p.add_argument('--name-prefix', metavar='prefix', dest='prefix',
>                         default="dpdk",
>                         help="Prefix for workqueue name to mark for DPDK use [default: 'dpdk']")
> +    arg_p.add_argument('--wq-option', action='append',
> +                       help="Provide additional config option for queues (format 'x=y')")
>      arg_p.add_argument('--reset', action='store_true',
>                         help="Reset DSA device and its queues")
>      parsed_args = arg_p.parse_args(args[1:])
> @@ -121,7 +142,7 @@ def main(args):
>      if parsed_args.reset:
>          reset_device(dsa_id)
>      else:
> -        configure_dsa(dsa_id, parsed_args.q, parsed_args.prefix)
> +        configure_dsa(dsa_id, parsed_args)
>  
>  
>  if __name__ == "__main__":
> -- 
> 2.31.1
> 

[PATCH v3] dma/idxd: add generic option for queue config

[Kevin Laatz]

The device config script currently uses some defaults to configure
devices in a generic way.

With the addition of this option, users have more control over how
queues are configured.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/dpdk_idxd_cfg.py | 34 +++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
index 3f5d5ee752..77c473b240 100755
--- a/drivers/dma/idxd/dpdk_idxd_cfg.py
+++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
@@ -63,7 +63,15 @@ def get_dsa_id(pci):
     sys.exit(f"Could not get device ID for device {pci}")
 
 
-def configure_dsa(dsa_id, queues, prefix):
+def parse_wq_opts(wq_opts):
+    "Parse user-specified queue configuration, creating a dict of options"
+    try:
+        return {o.split('=')[0]: o.split('=')[1] for o in wq_opts}
+    except ValueError:
+        sys.exit("Invalid --wq-option format, use format 'option=value'")
+
+
+def configure_dsa(dsa_id, args):
     "Configure the DSA instance with appropriate number of queues"
     dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
 
@@ -72,8 +80,8 @@ def configure_dsa(dsa_id, queues, prefix):
     max_queues = dsa_dir.read_int("max_work_queues")
     max_work_queues_size = dsa_dir.read_int("max_work_queues_size")
 
-    nb_queues = min(queues, max_queues)
-    if queues > nb_queues:
+    nb_queues = min(args.q, max_queues)
+    if args.q > nb_queues:
         print(f"Setting number of queues to max supported value: {max_queues}")
 
     # we want one engine per group, and no more engines than queues
@@ -83,14 +91,16 @@ def configure_dsa(dsa_id, queues, prefix):
 
     # configure each queue
     for q in range(nb_queues):
+        wqcfg = {"group_id": q % nb_groups,
+                 "type": "user",
+                 "mode": "dedicated",
+                 "name": f"{args.prefix}_wq{dsa_id}.{q}",
+                 "priority": 1,
+                 "max_batch_size": 1024,
+                 "size": int(max_work_queues_size / nb_queues)}
+        wqcfg.update(parse_wq_opts(args.wq_option))
         wq_dir = SysfsDir(os.path.join(dsa_dir.path, f"wq{dsa_id}.{q}"))
-        wq_dir.write_values({"group_id": q % nb_groups,
-                             "type": "user",
-                             "mode": "dedicated",
-                             "name": f"{prefix}_wq{dsa_id}.{q}",
-                             "priority": 1,
-                             "max_batch_size": 1024,
-                             "size": int(max_work_queues_size / nb_queues)})
+        wq_dir.write_values(wqcfg)
 
     # enable device and then queues
     idxd_dir = SysfsDir(get_drv_dir("idxd"))
@@ -112,6 +122,8 @@ def main(args):
     arg_p.add_argument('--name-prefix', metavar='prefix', dest='prefix',
                        default="dpdk",
                        help="Prefix for workqueue name to mark for DPDK use [default: 'dpdk']")
+    arg_p.add_argument('--wq-option', action='append',
+                       help="Provide additional config option for queues (format 'x=y')")
     arg_p.add_argument('--reset', action='store_true',
                        help="Reset DSA device and its queues")
     parsed_args = arg_p.parse_args(args[1:])
@@ -121,7 +133,7 @@ def main(args):
     if parsed_args.reset:
         reset_device(dsa_id)
     else:
-        configure_dsa(dsa_id, parsed_args.q, parsed_args.prefix)
+        configure_dsa(dsa_id, parsed_args)
 
 
 if __name__ == "__main__":
-- 
2.31.1

Re: [PATCH v3] dma/idxd: add generic option for queue config

[Bruce Richardson]

On Fri, Apr 01, 2022 at 11:35:00AM +0100, Kevin Laatz wrote:
> The device config script currently uses some defaults to configure
> devices in a generic way.
> 
> With the addition of this option, users have more control over how
> queues are configured.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

RE: [PATCH v3] dma/idxd: add generic option for queue config

[Pai G, Sunil]

> -----Original Message-----
> From: Kevin Laatz <kevin.laatz@intel.com>
> Sent: Friday, April 1, 2022 4:05 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Laatz, Kevin
> <kevin.laatz@intel.com>
> Subject: [PATCH v3] dma/idxd: add generic option for queue config
> 
> The device config script currently uses some defaults to configure devices
> in a generic way.
> 
> With the addition of this option, users have more control over how queues
> are configured.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/dpdk_idxd_cfg.py | 34 +++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 

Tried setting few capabilities of a dedicated WQ, works as expected.

Tested-by: Sunil Pai G <sunil.pai.g@intel.com>

Re: [PATCH v3] dma/idxd: add generic option for queue config

[Thomas Monjalon]

01/04/2022 12:51, Bruce Richardson:
> On Fri, Apr 01, 2022 at 11:35:00AM +0100, Kevin Laatz wrote:
> > The device config script currently uses some defaults to configure
> > devices in a generic way.
> > 
> > With the addition of this option, users have more control over how
> > queues are configured.
> > 
> > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> > ---
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks.