[v3,4/4] regex/mlx5: prevent wrong calculation of free sqs in umr mode
Checks
Commit Message
From: John Hurley <jhurley@nvidia.com>
A recent change adds support for scattered mbuf and UMR support for regex.
Part of this commit makes the pi and ci counters of the regex_sq a quarter
of the length in non umr mode, effectively moving them from 16 bits to
14. The new get_free method casts the difference in pi and ci to a 16 bit
value when calculating the free send queues, accounting for any wrapping
when pi has looped back to 0 but ci has not yet. However, the move to 14
bits while still casting to 16 can now lead to corrupted, large values
returned.
Modify the get_free function to take in the has_umr flag and, accordingly,
account for wrapping on either 14 or 16 bit pi/ci difference.
Fixes: d55c9f637263 ("regex/mlx5: add data path scattered mbuf process")
Signed-off-by: John Hurley <jhurley@nvidia.com>
Acked-by: Ori Kam <orika@nvidia.com>
---
drivers/regex/mlx5/mlx5_regex_fastpath.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
Comments
30/03/2021 03:39, Suanming Mou:
> From: John Hurley <jhurley@nvidia.com>
>
> A recent change adds support for scattered mbuf and UMR support for regex.
> Part of this commit makes the pi and ci counters of the regex_sq a quarter
> of the length in non umr mode, effectively moving them from 16 bits to
> 14. The new get_free method casts the difference in pi and ci to a 16 bit
> value when calculating the free send queues, accounting for any wrapping
> when pi has looped back to 0 but ci has not yet. However, the move to 14
> bits while still casting to 16 can now lead to corrupted, large values
> returned.
>
> Modify the get_free function to take in the has_umr flag and, accordingly,
> account for wrapping on either 14 or 16 bit pi/ci difference.
>
> Fixes: d55c9f637263 ("regex/mlx5: add data path scattered mbuf process")
It is fixing a patch in this series, right?
Why not squashing them?
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 7, 2021 12:23 AM
> To: John Hurley <jhurley@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; dev@dpdk.org; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] regex/mlx5: prevent wrong calculation
> of free sqs in umr mode
>
> 30/03/2021 03:39, Suanming Mou:
> > From: John Hurley <jhurley@nvidia.com>
> >
> > A recent change adds support for scattered mbuf and UMR support for regex.
> > Part of this commit makes the pi and ci counters of the regex_sq a
> > quarter of the length in non umr mode, effectively moving them from 16
> > bits to 14. The new get_free method casts the difference in pi and ci
> > to a 16 bit value when calculating the free send queues, accounting
> > for any wrapping when pi has looped back to 0 but ci has not yet.
> > However, the move to 14 bits while still casting to 16 can now lead to
> > corrupted, large values returned.
> >
> > Modify the get_free function to take in the has_umr flag and,
> > accordingly, account for wrapping on either 14 or 16 bit pi/ci difference.
> >
> > Fixes: d55c9f637263 ("regex/mlx5: add data path scattered mbuf
> > process")
>
> It is fixing a patch in this series, right?
> Why not squashing them?
Yes, this is a fix for this series.
This fix was done by John when he tested the code, so I put it as an individual one.
Should we update an new version to squash it?
(And Thomas, the latest version of this series is v4, you comment in this old v3 version now :) )
>
>
07/04/2021 03:00, Suanming Mou:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 30/03/2021 03:39, Suanming Mou:
> > > From: John Hurley <jhurley@nvidia.com>
> > >
> > > A recent change adds support for scattered mbuf and UMR support for regex.
> > > Part of this commit makes the pi and ci counters of the regex_sq a
> > > quarter of the length in non umr mode, effectively moving them from 16
> > > bits to 14. The new get_free method casts the difference in pi and ci
> > > to a 16 bit value when calculating the free send queues, accounting
> > > for any wrapping when pi has looped back to 0 but ci has not yet.
> > > However, the move to 14 bits while still casting to 16 can now lead to
> > > corrupted, large values returned.
> > >
> > > Modify the get_free function to take in the has_umr flag and,
> > > accordingly, account for wrapping on either 14 or 16 bit pi/ci difference.
> > >
> > > Fixes: d55c9f637263 ("regex/mlx5: add data path scattered mbuf
> > > process")
> >
> > It is fixing a patch in this series, right?
> > Why not squashing them?
>
> Yes, this is a fix for this series.
> This fix was done by John when he tested the code, so I put it as an individual one.
> Should we update an new version to squash it?
Yes better to squash it in a v5, thanks.
> (And Thomas, the latest version of this series is v4, you comment in this old v3 version now :) )
Yes, sorry.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 7, 2021 3:12 PM
> To: John Hurley <jhurley@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>
> Cc: dev@dpdk.org; Ori Kam <orika@nvidia.com>; dev@dpdk.org; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad <matan@nvidia.com>;
> Raslan Darawsheh <rasland@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] regex/mlx5: prevent wrong calculation
> of free sqs in umr mode
>
> 07/04/2021 03:00, Suanming Mou:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 30/03/2021 03:39, Suanming Mou:
> > > > From: John Hurley <jhurley@nvidia.com>
> > > >
> > > > A recent change adds support for scattered mbuf and UMR support for
> regex.
> > > > Part of this commit makes the pi and ci counters of the regex_sq a
> > > > quarter of the length in non umr mode, effectively moving them
> > > > from 16 bits to 14. The new get_free method casts the difference
> > > > in pi and ci to a 16 bit value when calculating the free send
> > > > queues, accounting for any wrapping when pi has looped back to 0 but ci
> has not yet.
> > > > However, the move to 14 bits while still casting to 16 can now
> > > > lead to corrupted, large values returned.
> > > >
> > > > Modify the get_free function to take in the has_umr flag and,
> > > > accordingly, account for wrapping on either 14 or 16 bit pi/ci difference.
> > > >
> > > > Fixes: d55c9f637263 ("regex/mlx5: add data path scattered mbuf
> > > > process")
> > >
> > > It is fixing a patch in this series, right?
> > > Why not squashing them?
> >
> > Yes, this is a fix for this series.
> > This fix was done by John when he tested the code, so I put it as an individual
> one.
> > Should we update an new version to squash it?
>
> Yes better to squash it in a v5, thanks.
OK, I see, thanks. Will update later.
>
> > (And Thomas, the latest version of this series is v4, you comment in
> > this old v3 version now :) )
>
> Yes, sorry.
>
@@ -192,8 +192,10 @@ send_doorbell(struct mlx5_regex_priv *priv, struct mlx5_regex_sq *sq)
}
static inline int
-get_free(struct mlx5_regex_sq *sq) {
- return (sq_size_get(sq) - (uint16_t)(sq->pi - sq->ci));
+get_free(struct mlx5_regex_sq *sq, uint8_t has_umr) {
+ return (sq_size_get(sq) - ((sq->pi - sq->ci) &
+ (has_umr ? (MLX5_REGEX_MAX_WQE_INDEX >> 2) :
+ MLX5_REGEX_MAX_WQE_INDEX)));
}
static inline uint32_t
@@ -385,7 +387,7 @@ mlx5_regexdev_enqueue_gga(struct rte_regexdev *dev, uint16_t qp_id,
while ((sqid = ffs(queue->free_sqs))) {
sqid--; /* ffs returns 1 for bit 0 */
sq = &queue->sqs[sqid];
- nb_desc = get_free(sq);
+ nb_desc = get_free(sq, priv->has_umr);
if (nb_desc) {
/* The ops be handled can't exceed nb_ops. */
if (nb_desc > nb_left)
@@ -418,7 +420,7 @@ mlx5_regexdev_enqueue(struct rte_regexdev *dev, uint16_t qp_id,
while ((sqid = ffs(queue->free_sqs))) {
sqid--; /* ffs returns 1 for bit 0 */
sq = &queue->sqs[sqid];
- while (get_free(sq)) {
+ while (get_free(sq, priv->has_umr)) {
job_id = job_id_get(sqid, sq_size_get(sq), sq->pi);
prep_one(priv, queue, sq, ops[i], &queue->jobs[job_id]);
i++;