Re: [PATCH] net/mlx5: fix spurious CPU wakeups caused by invalid CQE

["Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Alexander,

________________________________
From: Alexander Kozyrev <akozyrev@nvidia.com>
Sent: Tuesday, November 18, 2025 1:35 AM
To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>; Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
Cc: jerinj@marvell.com <jerinj@marvell.com>; kirankumark@marvell.com <kirankumark@marvell.com>; ndabilpuram@marvell.com <ndabilpuram@marvell.com>; yanzhirun_163@163.com <yanzhirun_163@163.com>; david.marchand@redhat.com <david.marchand@redhat.com>; ktraynor@redhat.com <ktraynor@redhat.com>; NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; konstantin.ananyev@huawei.com <konstantin.ananyev@huawei.com>; konstantin.v.ananyev@yandex.ru <konstantin.v.ananyev@yandex.ru>; bruce.richardson@intel.com <bruce.richardson@intel.com>; maxime.coquelin@redhat.com <maxime.coquelin@redhat.com>; anatoly.burakov@intel.com <anatoly.burakov@intel.com>; aconole@redhat.com <aconole@redhat.com>; dev@dpdk.org <dev@dpdk.org>; stable@dpdk.org <stable@dpdk.org>
Subject: Re: [PATCH] net/mlx5: fix spurious CPU wakeups caused by invalid CQE

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

>>> Fixes: a8f0df6bf98d ("net/mlx5: support power monitoring")
>>> Cc: akozyrev@nvidia.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>>> ---
>>>  drivers/net/mlx5/mlx5_rx.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c
>>> index 420a03068d..2765b4b730 100644
>>> --- a/drivers/net/mlx5/mlx5_rx.c
>>> +++ b/drivers/net/mlx5/mlx5_rx.c
>>> @@ -295,6 +295,20 @@ mlx5_monitor_callback(const uint64_t value,
>>>       return (value & m) == v ? -1 : 0;
>>>  }
>>>
>>> +static int
>>> +mlx5_monitor_cqe_own_callback(const uint64_t value,
>>> +             const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])
>>> +{
>>> +     const uint64_t m = opaque[CLB_MSK_IDX];
>>> +     const uint64_t v = opaque[CLB_VAL_IDX];
>>> +     const uint64_t match = ((value & m) == v);
>>
>> Could you please rename "match" variable to "sw_owned"?
>> This name would better relay the meaning of the checked condition that
>> CQE owner bit value signifies that CQE is SW owned.
>ACK! Will update this in v2.
>>
>>> +     const uint64_t opcode = MLX5_CQE_OPCODE(value);
>>> +     const uint64_t valid_op = (opcode ^ MLX5_CQE_INVALID);
>>
>>IMO the usage of bit operations here (although logic is correct) is a bit confusing.
>>Could you rewrite it in terms of logical operations so it's easier to
>>follow? For example like this:
>>
>>        const uint64_t valid_op = opcode != MLX5_CQE_INVALID
>>
>>        return (sw_owned && valid_op) ? -1 : 0;
>>
>>This also would properly describe in code the required condition:
>>CQE can be parsed by SW if and only if owner bit is "SW owned" and CQE
>>opcode is valid.
>ACK! Will update this in v2.
>>
>>> +
>>> +     /* ownership bit is not valid for invalid opcode; CQE is HW owned */
>>> +     return -(match & valid_op);
>>> +}
>>> +
>>> int mlx5_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)
>>>  {
>>>       struct mlx5_rxq_data *rxq = rx_queue;
>>> @@ -312,12 +326,13 @@ int mlx5_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)
>>>               pmc->addr = &cqe->validity_iteration_count;
>>>               pmc->opaque[CLB_VAL_IDX] = vic;
>>>               pmc->opaque[CLB_MSK_IDX] = MLX5_CQE_VIC_INIT;
>>> +             pmc->fn = mlx5_monitor_callback;
>>
>>Alex, Slava: Just to double check - in case of enhanced CQE compression
>>layout, should both CQE opcode and vic be checked?
>>Right now only vic is checked in power monitor callback for that case.
>>In Rx datapath both are checked to determine CQE ownership:
>>https://github.com/DPDK/dpdk/blob/main/drivers/common/mlx5/mlx5_common.h#L277
>
>Sorry for the late reply. I think we should check opcode in both cases.
>mlx5_monitor_callback can be updated with the opcode check for enhanced CQE compression layout,
>instead of having 2 separate callback functions. Could you please prepare a follow-up patch for that?
Ok, I can extend this patch to also cover for enhanced CQE compression case as well.
Right now, the new call back was added to avoid additional checks in the older callback function.
I can rework on this as needed.
>>
>>>       } else {
>>>               pmc->addr = &cqe->op_own;
>>>               pmc->opaque[CLB_VAL_IDX] = !!idx;
>>>               pmc->opaque[CLB_MSK_IDX] = MLX5_CQE_OWNER_MASK;
>>> +             pmc->fn = mlx5_monitor_cqe_own_callback;
>>>       }
>>> -     pmc->fn = mlx5_monitor_callback;
>>>       pmc->size = sizeof(uint8_t);
>>>       return 0;
>>>  }
>>> --
>>> 2.43.0
>>>
>>

[-- Attachment #2: Type: text/html, Size: 10336 bytes --]