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

[Alexander Kozyrev <akozyrev@nvidia.com>]

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

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

>
>>       } 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: 6451 bytes --]