Re: [PATCH 1/2] baseband/fpga_5gnr_fec: use new barrier API

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Tyler,

On 2/26/24 12:03, Maxime Coquelin wrote:
> Hello,
> 
> On 2/22/24 19:05, Chautru, Nicolas wrote:
>> Hi Maxime,
>>
>> Why would we change this here and now? Is the intent not to use new 
>> suggested semantics for new patches only?
> 
> The pull request was rejected because of the use of such barrier, which 
> is reported by checkpatch.
> 
> ### [PATCH] baseband/fpga_5gnr_fec: add AGX100 support
> Warning in drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c:
> Using rte_smp_[r/w]mb
> 
>> Are all DPDK drivers being changed?
> 
> My understanding is that for now, only new occurrences are prohibited,
> can you confirm Tyler?

Could you please clarify whether conversion for existing rte_smp_rmb()
calls to rte_atomic_thread_fence() in drivers is planned, or only new
occurrences are prohibited?

Thanks in advances,
Maxime

> If so we could only change for now the patch adding ACX100.
> But... I preferred doing the changes for all bbdev drivers for
> consistency.
> 
>> I am unsure we would want to change these drivers, this is kind of 
>> risk introduced by code churn that gets ecosystem unwilling to move to 
>> latest version.
> 
> I think it is better to change now that we are far from the next LTS.
> 
>> These memory barriers issues are awful to troubleshoot or properly 
>> validate, so personally quite reluctant to change.
> 
> If I disassemble fpga_dequeue_enc() with and without the patch, I cannot
> spot a difference.
> 
> Thomas, are you waiting for this series to be applied to take the pull
> request that was initially for -rc1?
> 
> Thanks,
> Maxime
> 
>> Thanks
>> Nic
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Thursday, February 22, 2024 8:21 AM
>>> To: dev@dpdk.org; Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas,
>>> Hernan <hernan.vargas@intel.com>; Marchand, David
>>> <david.marchand@redhat.com>; thomas@monjalon.net;
>>> roretzla@linux.microsoft.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [PATCH 1/2] baseband/fpga_5gnr_fec: use new barrier API
>>>
>>> rte_smp_rmb() is deprecated, use the new API instead as suggested in
>>> rte_atomic header.
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>   drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> index efc1d3a772..314c87350e 100644
>>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> @@ -2661,7 +2661,7 @@ vc_5gnr_dequeue_ldpc_enc_one_op_cb(struct
>>> fpga_5gnr_queue *q, struct rte_bbdev_e
>>>           return -1;
>>>
>>>       /* make sure the response is read atomically */
>>> -    rte_smp_rmb();
>>> +    rte_atomic_thread_fence(rte_memory_order_acquire);
>>>
>>>       rte_bbdev_log_debug("DMA response desc %p", desc);
>>>
>>> @@ -2690,7 +2690,7 @@ agx100_dequeue_ldpc_enc_one_op_cb(struct
>>> fpga_5gnr_queue *q, struct rte_bbdev_en
>>>           return -1;
>>>
>>>       /* make sure the response is read atomically. */
>>> -    rte_smp_rmb();
>>> +    rte_atomic_thread_fence(rte_memory_order_acquire);
>>>
>>>       rte_bbdev_log_debug("DMA response desc %p", desc);
>>>
>>> @@ -2722,7 +2722,7 @@ vc_5gnr_dequeue_ldpc_dec_one_op_cb(struct
>>> fpga_5gnr_queue *q, struct rte_bbdev_d
>>>           return -1;
>>>
>>>       /* make sure the response is read atomically */
>>> -    rte_smp_rmb();
>>> +    rte_atomic_thread_fence(rte_memory_order_acquire);
>>>
>>>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>>>       vc_5gnr_print_dma_dec_desc_debug_info(desc);
>>> @@ -2768,7 +2768,7 @@ agx100_dequeue_ldpc_dec_one_op_cb(struct
>>> fpga_5gnr_queue *q, struct rte_bbdev_de
>>>           return -1;
>>>
>>>       /* make sure the response is read atomically. */
>>> -    rte_smp_rmb();
>>> +    rte_atomic_thread_fence(rte_memory_order_acquire);
>>>
>>>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>>>       agx100_print_dma_dec_desc_debug_info(desc);
>>> -- 
>>> 2.43.0
>>