<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
>> Fixes: a8f0df6bf98d ("net/mlx5: support power monitoring")<br>
>> Cc: akozyrev@nvidia.com<br>
>> Cc: stable@dpdk.org<br>
>><br>
>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com><br>
>> ---<br>
>> drivers/net/mlx5/mlx5_rx.c | 17 ++++++++++++++++-<br>
>> 1 file changed, 16 insertions(+), 1 deletion(-)<br>
>><br>
>> diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c<br>
>> index 420a03068d..2765b4b730 100644<br>
>> --- a/drivers/net/mlx5/mlx5_rx.c<br>
>> +++ b/drivers/net/mlx5/mlx5_rx.c<br>
>> @@ -295,6 +295,20 @@ mlx5_monitor_callback(const uint64_t value,<br>
>> return (value & m) == v ? -1 : 0;<br>
>> }<br>
>><br>
>> +static int<br>
>> +mlx5_monitor_cqe_own_callback(const uint64_t value,<br>
>> + const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])<br>
>> +{<br>
>> + const uint64_t m = opaque[CLB_MSK_IDX];<br>
>> + const uint64_t v = opaque[CLB_VAL_IDX];<br>
>> + const uint64_t match = ((value & m) == v);<br>
><br>
> Could you please rename "match" variable to "sw_owned"?<br>
> This name would better relay the meaning of the checked condition that<br>
> CQE owner bit value signifies that CQE is SW owned.<br>
ACK! Will update this in v2.</div>
<div style="direction: ltr; font-size: 11pt;">><br>
>> + const uint64_t opcode = MLX5_CQE_OPCODE(value);<br>
>> + const uint64_t valid_op = (opcode ^ MLX5_CQE_INVALID);<br>
><br>
>IMO the usage of bit operations here (although logic is correct) is a bit confusing.<br>
>Could you rewrite it in terms of logical operations so it's easier to<br>
>follow? For example like this:<br>
><br>
> const uint64_t valid_op = opcode != MLX5_CQE_INVALID<br>
><br>
> return (sw_owned && valid_op) ? -1 : 0;<br>
><br>
>This also would properly describe in code the required condition:<br>
>CQE can be parsed by SW if and only if owner bit is "SW owned" and CQE<br>
>opcode is valid.</div>
<div style="direction: ltr; font-size: 11pt; color: black;">ACK! Will update this in v2.</div>
<div style="direction: ltr; font-size: 11pt;" class="elementToProof">><br>
>> +<br>
>> + /* ownership bit is not valid for invalid opcode; CQE is HW owned */<br>
>> + return -(match & valid_op);<br>
>> +}<br>
>> +<br>
>> int mlx5_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)<br>
>> {<br>
>> struct mlx5_rxq_data *rxq = rx_queue;<br>
>> @@ -312,12 +326,13 @@ int mlx5_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)<br>
>> pmc->addr = &cqe->validity_iteration_count;<br>
>> pmc->opaque[CLB_VAL_IDX] = vic;<br>
>> pmc->opaque[CLB_MSK_IDX] = MLX5_CQE_VIC_INIT;<br>
>> + pmc->fn = mlx5_monitor_callback;<br>
><br>
>Alex, Slava: Just to double check - in case of enhanced CQE compression<br>
>layout, should both CQE opcode and vic be checked?<br>
>Right now only vic is checked in power monitor callback for that case.<br>
>In Rx datapath both are checked to determine CQE ownership:</div>
<div style="direction: ltr; font-size: 11pt;">><a data-auth="NotApplicable" class="OWAAutoLink" id="OWA9a0f3ebf-fbf5-a7c1-a2ee-b68156978848" href="https://github.com/DPDK/dpdk/blob/main/drivers/common/mlx5/mlx5_common.h#L277">https://github.com/DPDK/dpdk/blob/main/drivers/common/mlx5/mlx5_common.h#L277</a></div>
<div style="direction: ltr; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<div style="direction: ltr; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
Sorry for the late reply. I think we should check opcode in both cases.</div>
<div style="direction: ltr; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
mlx5_monitor_callback can be updated with the opcode check for enhanced CQE compression layout,</div>
<div style="direction: ltr; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
instead of having 2 separate callback functions. Could you please prepare a follow-up patch for that?</div>
<div style="direction: ltr; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<div style="direction: ltr; font-size: 11pt;">><br>
>> } else {<br>
>> pmc->addr = &cqe->op_own;<br>
>> pmc->opaque[CLB_VAL_IDX] = !!idx;<br>
>> pmc->opaque[CLB_MSK_IDX] = MLX5_CQE_OWNER_MASK;<br>
>> + pmc->fn = mlx5_monitor_cqe_own_callback;<br>
>> }<br>
>> - pmc->fn = mlx5_monitor_callback;<br>
>> pmc->size = sizeof(uint8_t);<br>
>> return 0;<br>
>> }<br>
>> --<br>
>> 2.43.0<br>
>><br>
><br>
</div>
</body>
</html>