[PATCH] dma/idxd: fix burst capacity calculation

[PATCH] dma/idxd: fix burst capacity calculation

[Bruce Richardson]

When the maximum burst size supported by HW is less than the available
ring space, incorrect capacity was returned when there was already some
jobs queued up for submission. This was because the capacity calculation
failed to subtract the number of already-enqueued jobs from the max
burst size. After subtraction is done, ensure that any negative values
(which should never occur if the user respects the reported limits), are
clamped to zero.

Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
Cc: kevin.laatz@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index fc11b11337..4442d1cbbd 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -485,7 +485,9 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
 		write_idx += idxd->desc_ring_mask + 1;
 	used_space = write_idx - idxd->ids_returned;

-	return RTE_MIN((idxd->desc_ring_mask - used_space), idxd->max_batch_size);
+	const int ret = RTE_MIN((idxd->desc_ring_mask - used_space),
+			(idxd->max_batch_size - idxd->batch_size));
+	return ret < 0 ? 0 : (uint16_t)ret;
 }

 int
--
2.32.0

Re: [PATCH] dma/idxd: fix burst capacity calculation

[Kevin Laatz]

On 20/12/2021 17:05, Bruce Richardson wrote:
> When the maximum burst size supported by HW is less than the available
> ring space, incorrect capacity was returned when there was already some
> jobs queued up for submission. This was because the capacity calculation
> failed to subtract the number of already-enqueued jobs from the max
> burst size. After subtraction is done, ensure that any negative values
> (which should never occur if the user respects the reported limits), are
> clamped to zero.
>
> Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
> Cc: kevin.laatz@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/idxd/idxd_common.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

RE: [PATCH] dma/idxd: fix burst capacity calculation

[Hu, Jiayu]

Tested-by: Jiayu Hu <jiayu.hu@intel.com>

> -----Original Message-----
> From: Kevin Laatz <kevin.laatz@intel.com>
> Sent: Wednesday, January 5, 2022 1:17 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH] dma/idxd: fix burst capacity calculation
> 
> 
> On 20/12/2021 17:05, Bruce Richardson wrote:
> > When the maximum burst size supported by HW is less than the available
> > ring space, incorrect capacity was returned when there was already
> > some jobs queued up for submission. This was because the capacity
> > calculation failed to subtract the number of already-enqueued jobs
> > from the max burst size. After subtraction is done, ensure that any
> > negative values (which should never occur if the user respects the
> > reported limits), are clamped to zero.
> >
> > Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
> > Cc: kevin.laatz@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   drivers/dma/idxd/idxd_common.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> Acked-by: Kevin Laatz <kevin.laatz@intel.com>

RE: [PATCH] dma/idxd: fix burst capacity calculation

[Pai G, Sunil]

Hi Bruce, Kevin

This patch seems to have uncovered a bug in the driver.
On applying, the idxd_burst_capacity API seems to return 0 for cases even when there are batch descriptors and ring space available.
Seems like there is a wraparound missing when calculating the descriptor ring space, causing this behavior.  

Below change seems to fix the issue.

@@ -483,7 +496,7 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
        /* For descriptors, check for wrap-around on write but not read */                   
        if (idxd->ids_returned > write_idx)                                                  
                write_idx += idxd->desc_ring_mask + 1;                                       
-       used_space = write_idx - idxd->ids_returned;                                         
+       used_space = (write_idx - idxd->ids_returned)&idxd->desc_ring_mask;                  

<snipped>

Could we include this fix in the current patch ?

Thanks and regards,
Sunil

Re: [PATCH] dma/idxd: fix burst capacity calculation

[Bruce Richardson]

On Mon, Jan 10, 2022 at 01:09:02PM +0000, Pai G, Sunil wrote:
> Hi Bruce, Kevin
> 
> This patch seems to have uncovered a bug in the driver.
> On applying, the idxd_burst_capacity API seems to return 0 for cases even when there are batch descriptors and ring space available.
> Seems like there is a wraparound missing when calculating the descriptor ring space, causing this behavior.
> 
> Below change seems to fix the issue.
> 
> @@ -483,7 +496,7 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
>         /* For descriptors, check for wrap-around on write but not read */
>         if (idxd->ids_returned > write_idx)
>                 write_idx += idxd->desc_ring_mask + 1;
> -       used_space = write_idx - idxd->ids_returned;
> +       used_space = (write_idx - idxd->ids_returned)&idxd->desc_ring_mask;
> 
> <snipped>
> 
> Could we include this fix in the current patch ?
>
Hi Sunil,

what values for the write_idx and ids_returned vars give this error, and
how does masking help? I'd expect masking to increase the number of times
the function returns zero, rather than decreasing it.

/Bruce 

RE: [PATCH] dma/idxd: fix burst capacity calculation

[Pai G, Sunil]

Hi Bruce,

> what values for the write_idx and ids_returned vars give this error, and how
> does masking help? I'd expect masking to increase the number of times the
> function returns zero, rather than decreasing it.


Here are the values from the idxd dump:
dev_capa: 0x500000051 - mem2mem sva handles_errors copy fill
max_vchans_supported: 1
nb_vchans_configured: 1
silent_mode: off
 IDXD Private Data ==
Portal: 0x7ffff7ffb000
Config: { ring_size: 4096 }
Batch ring (sz = 129, max_batches = 128):
62370  62402  62434  62466  62498  62530  62562  62594  62626  62658  62690  62722  62754  62786  62818  62850  62882  62914  62946  62978  63010  63042  63074  6
3106  63138  63170  63202  63234  63266  63298  63330  63362  63394  63426  63458  63490  63522  63554  63586  63618  63650  63682  63714  63746  63778  63810  63842  63874  63906  63938  63970  64002  64034  64066  64098  6413
0  64162  64194  64226  64258  64290  64322  64354  64386  64418  64450  64482  64514  64546  64578  64610  64642  64674  64706  64738  64770  64802  64834  64866  64898  64930  64962  64994  65026  65058  65090  65122  65154  
65186  65218  65250  65282  65314  65346  65378  65410  65442  65474  65506 [rd ptr]  2 [wr ptr]  61442  61474  61506  61538  61570  61602  61634  61666  61698  61730  61762  61794  61826  61858  61890  61922  61954  61986  620
18  62050  62082  62114  62146  62178  62210  62242  62274  62306  62338
  Curr batch: start = 2, size = 0
  IDS: avail = 65506, returned: 65506
max packets per batch from hw: 1024
batches left: 127, ring space left: 4064
idxd->desc_ring_mask: 4095, used_space: 4128, used_space: 4128, idxd->max_batch_size: 1024, idxd->batch_size: 0
write_idx: 4098, idxd->batch_idx_read: 98, idxd->batch_idx_write: 99, idxd->desc_ring_mask - used_space: 65503

relevant data from above:
write_idx: 4098 , IDS returned: 65506, idxd->desc_ring_mask: 4095

without the fix :
used_space = write_idx - idxd->ids_returned; (4098 - 65506)   = -61408

with fix: 
used_space = (write_idx - idxd->ids_returned)& idxd->desc_ring_mask; (4098 - 65506)&4095   = 32 
which seems to match the rd ptr and wr ptr.

Thanks and regards,
Sunil

Re: [PATCH] dma/idxd: fix burst capacity calculation

[Bruce Richardson]

On Mon, Jan 10, 2022 at 01:44:06PM +0000, Pai G, Sunil wrote:
> Hi Bruce,
> 
> > what values for the write_idx and ids_returned vars give this error, and how
> > does masking help? I'd expect masking to increase the number of times the
> > function returns zero, rather than decreasing it.
> 
> 
> Here are the values from the idxd dump:
> dev_capa: 0x500000051 - mem2mem sva handles_errors copy fill
> max_vchans_supported: 1
> nb_vchans_configured: 1
> silent_mode: off
>  IDXD Private Data ==
> Portal: 0x7ffff7ffb000
> Config: { ring_size: 4096 }
> Batch ring (sz = 129, max_batches = 128):
> 62370  62402  62434  62466  62498  62530  62562  62594  62626  62658  62690  62722  62754  62786  62818  62850  62882  62914  62946  62978  63010  63042  63074  6
> 3106  63138  63170  63202  63234  63266  63298  63330  63362  63394  63426  63458  63490  63522  63554  63586  63618  63650  63682  63714  63746  63778  63810  63842  63874  63906  63938  63970  64002  64034  64066  64098  6413
> 0  64162  64194  64226  64258  64290  64322  64354  64386  64418  64450  64482  64514  64546  64578  64610  64642  64674  64706  64738  64770  64802  64834  64866  64898  64930  64962  64994  65026  65058  65090  65122  65154
> 65186  65218  65250  65282  65314  65346  65378  65410  65442  65474  65506 [rd ptr]  2 [wr ptr]  61442  61474  61506  61538  61570  61602  61634  61666  61698  61730  61762  61794  61826  61858  61890  61922  61954  61986  620
> 18  62050  62082  62114  62146  62178  62210  62242  62274  62306  62338
>   Curr batch: start = 2, size = 0
>   IDS: avail = 65506, returned: 65506
> max packets per batch from hw: 1024
> batches left: 127, ring space left: 4064
> idxd->desc_ring_mask: 4095, used_space: 4128, used_space: 4128, idxd->max_batch_size: 1024, idxd->batch_size: 0
> write_idx: 4098, idxd->batch_idx_read: 98, idxd->batch_idx_write: 99, idxd->desc_ring_mask - used_space: 65503
> 
> relevant data from above:
> write_idx: 4098 , IDS returned: 65506, idxd->desc_ring_mask: 4095
> 
> without the fix :
> used_space = write_idx - idxd->ids_returned; (4098 - 65506)   = -61408
> 
> with fix:
> used_space = (write_idx - idxd->ids_returned)& idxd->desc_ring_mask; (4098 - 65506)&4095   = 32
> which seems to match the rd ptr and wr ptr.
> 
Thanks, Sunil, that's clear now.
Rather than clamping at the end, I think it may be more logical to clamp
the ids_returned value at the start instead. How about the following diff -
does that also fix it for you?

/Bruce

--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -472,6 +472,7 @@ uint16_t
 idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
 {
        const struct idxd_dmadev *idxd = dev_private;
+       const uint16_t read_idx = idxd->ids_returned & idxd->desc_ring_mask;
        uint16_t write_idx = idxd->batch_start + idxd->batch_size;
        uint16_t used_space;
 
@@ -481,9 +482,9 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
                return 0;
 
        /* For descriptors, check for wrap-around on write but not read */
-       if (idxd->ids_returned > write_idx)
+       if (read_idx > write_idx)
                write_idx += idxd->desc_ring_mask + 1;
-       used_space = write_idx - idxd->ids_returned;
+       used_space = write_idx - read_idx;
 
        return RTE_MIN((idxd->desc_ring_mask - used_space), idxd->max_batch_size);
 }

[PATCH v2 1/4] dma/idxd: fix burst capacity calculation

[Bruce Richardson]

When the maximum burst size supported by HW is less than the available
ring space, incorrect capacity was returned when there was already some
jobs queued up for submission. This was because the capacity calculation
failed to subtract the number of already-enqueued jobs from the max
burst size. After subtraction is done, ensure that any negative values
(which should never occur if the user respects the reported limits), are
clamped to zero.

Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
Cc: kevin.laatz@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>
Tested-by: Jiayu Hu <jiayu.hu@intel.com>
---
 drivers/dma/idxd/idxd_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index fc11b11337..4442d1cbbd 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -485,7 +485,9 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
 		write_idx += idxd->desc_ring_mask + 1;
 	used_space = write_idx - idxd->ids_returned;
 
-	return RTE_MIN((idxd->desc_ring_mask - used_space), idxd->max_batch_size);
+	const int ret = RTE_MIN((idxd->desc_ring_mask - used_space),
+			(idxd->max_batch_size - idxd->batch_size));
+	return ret < 0 ? 0 : (uint16_t)ret;
 }
 
 int
-- 
2.32.0

[PATCH v2 2/4] dma/idxd: fix paths to driver sysfs directory

[Bruce Richardson]

Recent kernel changes[1][2] mean that we cannot guarantee that the paths
in sysfs used for creating/binding a DSA or workqueue instance will be
as given in the utility script, since they are now "compatibility-mode
only". Update script to support both new paths and compatibility ones.

[1] https://lore.kernel.org/all/162637445139.744545.6008938867943724701.stgit@djiang5-desk3.ch.intel.com/
[2] https://lore.kernel.org/all/162637468705.744545.4399080971745974435.stgit@djiang5-desk3.ch.intel.com/

Fixes: 01863b9d2354 ("raw/ioat: include example configuration script")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/dpdk_idxd_cfg.py | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/dpdk_idxd_cfg.py b/drivers/dma/idxd/dpdk_idxd_cfg.py
index fcc27822ef..34537cb980 100755
--- a/drivers/dma/idxd/dpdk_idxd_cfg.py
+++ b/drivers/dma/idxd/dpdk_idxd_cfg.py
@@ -29,9 +29,17 @@ def write_values(self, values):
                 f.write(str(contents))
 
 
+def get_drv_dir(dtype):
+    "Get the sysfs path for the driver, either 'idxd' or 'user'"
+    drv_dir = "/sys/bus/dsa/drivers/" + dtype
+    if not os.path.exists(drv_dir):
+        return "/sys/bus/dsa/drivers/dsa"
+    return drv_dir
+
+
 def reset_device(dsa_id):
     "Reset the DSA device and all its queues"
-    drv_dir = SysfsDir("/sys/bus/dsa/drivers/dsa")
+    drv_dir = SysfsDir(get_drv_dir("idxd"))
     drv_dir.write_values({"unbind": f"dsa{dsa_id}"})
 
 
@@ -58,7 +66,6 @@ def get_dsa_id(pci):
 def configure_dsa(dsa_id, queues, prefix):
     "Configure the DSA instance with appropriate number of queues"
     dsa_dir = SysfsDir(f"/sys/bus/dsa/devices/dsa{dsa_id}")
-    drv_dir = SysfsDir("/sys/bus/dsa/drivers/dsa")
 
     max_groups = dsa_dir.read_int("max_groups")
     max_engines = dsa_dir.read_int("max_engines")
@@ -85,9 +92,12 @@ def configure_dsa(dsa_id, queues, prefix):
                              "size": int(max_work_queues_size / nb_queues)})
 
     # enable device and then queues
-    drv_dir.write_values({"bind": f"dsa{dsa_id}"})
+    idxd_dir = SysfsDir(get_drv_dir("idxd"))
+    idxd_dir.write_values({"bind": f"dsa{dsa_id}"})
+
+    user_dir = SysfsDir(get_drv_dir("user"))
     for q in range(nb_queues):
-        drv_dir.write_values({"bind": f"wq{dsa_id}.{q}"})
+        user_dir.write_values({"bind": f"wq{dsa_id}.{q}"})
 
 
 def main(args):
-- 
2.32.0

Re: [PATCH v2 2/4] dma/idxd: fix paths to driver sysfs directory

[Kevin Laatz]

On 11/01/2022 13:41, Bruce Richardson wrote:
> Recent kernel changes[1][2] mean that we cannot guarantee that the paths
> in sysfs used for creating/binding a DSA or workqueue instance will be
> as given in the utility script, since they are now "compatibility-mode
> only". Update script to support both new paths and compatibility ones.
>
> [1] https://lore.kernel.org/all/162637445139.744545.6008938867943724701.stgit@djiang5-desk3.ch.intel.com/
> [2] https://lore.kernel.org/all/162637468705.744545.4399080971745974435.stgit@djiang5-desk3.ch.intel.com/
>
> Fixes: 01863b9d2354 ("raw/ioat: include example configuration script")
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/idxd/dpdk_idxd_cfg.py | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>

Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[PATCH v2 3/4] dma/idxd: fix wrap-around in burst capacity calculation

[Bruce Richardson]

The burst capacity calculation code assumes that the write and read
(i.e. ids_returned) values both wrap at the ring-size, but the read
value instead wraps as UINT16_MAX. Therefore, instead of just adding
ring-size to the write value in case the read is greater, we need to
just always mask the result to ensure a correct, in-range, value.

Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
Cc: kevin.laatz@intel.com
Cc: stable@dpdk.org

Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_common.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index 4442d1cbbd..ea6413cc7a 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -480,10 +480,8 @@ idxd_burst_capacity(const void *dev_private, uint16_t vchan __rte_unused)
 			idxd->batch_idx_write + 1 == idxd->batch_idx_read)
 		return 0;
 
-	/* For descriptors, check for wrap-around on write but not read */
-	if (idxd->ids_returned > write_idx)
-		write_idx += idxd->desc_ring_mask + 1;
-	used_space = write_idx - idxd->ids_returned;
+	/* Subtract and mask to get in correct range */
+	used_space = (write_idx - idxd->ids_returned) & idxd->desc_ring_mask;
 
 	const int ret = RTE_MIN((idxd->desc_ring_mask - used_space),
 			(idxd->max_batch_size - idxd->batch_size));
-- 
2.32.0

RE: [PATCH v2 3/4] dma/idxd: fix wrap-around in burst capacity calculation

[Pai G, Sunil]

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

<snipped>

Thanks and regards,
Sunil

Re: [PATCH v2 3/4] dma/idxd: fix wrap-around in burst capacity calculation

[Kevin Laatz]

On 11/01/2022 13:41, Bruce Richardson wrote:
> The burst capacity calculation code assumes that the write and read
> (i.e. ids_returned) values both wrap at the ring-size, but the read
> value instead wraps as UINT16_MAX. Therefore, instead of just adding
> ring-size to the write value in case the read is greater, we need to
> just always mask the result to ensure a correct, in-range, value.
>
> Fixes: 9459de4edc99 ("dma/idxd: add burst capacity")
> Cc: kevin.laatz@intel.com
> Cc: stable@dpdk.org
>
> Reported-by: Sunil Pai G <sunil.pai.g@intel.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/dma/idxd/idxd_common.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>

Acked-by: Kevin Laatz <kevin.laatz@intel.com>