diff mbox series

[v2,38/45] bus/vmbus: use rte stdatomic API

Message ID 1711048652-7512-39-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series use stdatomic API |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Tyler Retzlaff March 21, 2024, 7:17 p.m. UTC 
  Replace the use of gcc builtin __atomic_xxx intrinsics with
corresponding rte_atomic_xxx optional rte stdatomic API.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/bus/vmbus/vmbus_channel.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Mattias Rönnblom March 21, 2024, 9:12 p.m. UTC | #1 
On 2024-03-21 20:17, Tyler Retzlaff wrote:
> Replace the use of gcc builtin __atomic_xxx intrinsics with
> corresponding rte_atomic_xxx optional rte stdatomic API.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>   drivers/bus/vmbus/vmbus_channel.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
> index 4d74df3..e96a4eb 100644
> --- a/drivers/bus/vmbus/vmbus_channel.c
> +++ b/drivers/bus/vmbus/vmbus_channel.c
> @@ -19,22 +19,23 @@
>   #include "private.h"
>   
>   static inline void
> -vmbus_sync_set_bit(volatile uint32_t *addr, uint32_t mask)
> +vmbus_sync_set_bit(volatile RTE_ATOMIC(uint32_t) *addr, uint32_t mask)
>   {
>   	/* Use GCC builtin which atomic does atomic OR operation */

Remove/rephrase this comment.

> -	__atomic_fetch_or(addr, mask, __ATOMIC_SEQ_CST);
> +	rte_atomic_fetch_or_explicit(addr, mask, rte_memory_order_seq_cst);
>   }
>   
>   static inline void
>   vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t monitor_id)
>   {
> -	uint32_t *monitor_addr, monitor_mask;
> +	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;
>   	unsigned int trigger_index;
>   
>   	trigger_index = monitor_id / HV_MON_TRIG_LEN;
>   	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
>   
> -	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
> +	monitor_addr =
> +	    (uint32_t __rte_atomic *)&channel->monitor_page->trigs[trigger_index].pending;

Why is "pending" not RTE_ATOMIC()?

>   	vmbus_sync_set_bit(monitor_addr, monitor_mask);
>   }
>
Long Li March 21, 2024, 9:34 p.m. UTC | #2 
> >   static inline void
> >   vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t
> monitor_id)
> >   {
> > -	uint32_t *monitor_addr, monitor_mask;
> > +	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;
> >   	unsigned int trigger_index;
> >
> >   	trigger_index = monitor_id / HV_MON_TRIG_LEN;
> >   	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
> >
> > -	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
> > +	monitor_addr =
> > +	    (uint32_t __rte_atomic
> > +*)&channel->monitor_page->trigs[trigger_index].pending;
> 
> Why is "pending" not RTE_ATOMIC()?

The usage is okay. The value is used to notify the VSP (Hyper-V). It's always set (no read) from DPDK.

Linux kernel driver does the same thing.

Long
Mattias Rönnblom March 22, 2024, 7:04 a.m. UTC | #3 
On 2024-03-21 22:34, Long Li wrote:
> 
>>>    static inline void
>>>    vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t
>> monitor_id)
>>>    {
>>> -	uint32_t *monitor_addr, monitor_mask;
>>> +	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;
>>>    	unsigned int trigger_index;
>>>
>>>    	trigger_index = monitor_id / HV_MON_TRIG_LEN;
>>>    	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
>>>
>>> -	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
>>> +	monitor_addr =
>>> +	    (uint32_t __rte_atomic
>>> +*)&channel->monitor_page->trigs[trigger_index].pending;
>>
>> Why is "pending" not RTE_ATOMIC()?
> 
> The usage is okay. The value is used to notify the VSP (Hyper-V). It's always set (no read) from DPDK.
> 

OK, so my question was not "does it need to be atomic", but rather "why 
isn't it marked RTE_ATOMIC() when it's treated as atomic".

But what you are saying is that it need not be atomic? Just the 
equivalent of WRITE_ONCE()? Or a relaxed atomic store?

> Linux kernel driver does the same thing.
> 
> Long
Long Li March 22, 2024, 7:32 p.m. UTC | #4 
> > The usage is okay. The value is used to notify the VSP (Hyper-V). It's always set
> (no read) from DPDK.
> >
> 
> OK, so my question was not "does it need to be atomic", but rather "why isn't it
> marked RTE_ATOMIC() when it's treated as atomic".
> 
> But what you are saying is that it need not be atomic? Just the equivalent of
> WRITE_ONCE()? Or a relaxed atomic store?

Sorry I misunderstood your question. Yes, it will be a good idea to make "pending" as RTE_ATOMIC.

This value needs to be atomic. However, the existing code is still correct in that updating is done in atomic.
Long Li March 22, 2024, 7:34 p.m. UTC | #5 
>  static inline void
>  vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t monitor_id)
> {
> -	uint32_t *monitor_addr, monitor_mask;
> +	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;

Does this mean monitor_mask will also change to RTE_ATOMIC(uint32_t)? 

Seems not necessary.

>  	unsigned int trigger_index;
> 
>  	trigger_index = monitor_id / HV_MON_TRIG_LEN;
>  	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
> 
> -	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
> +	monitor_addr =
> +	    (uint32_t __rte_atomic
> +*)&channel->monitor_page->trigs[trigger_index].pending;
>  	vmbus_sync_set_bit(monitor_addr, monitor_mask);  }
> 
> --
> 1.8.3.1
Tyler Retzlaff March 25, 2024, 4:41 p.m. UTC | #6 
On Fri, Mar 22, 2024 at 07:34:46PM +0000, Long Li wrote:
> >  static inline void
> >  vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t monitor_id)
> > {
> > -	uint32_t *monitor_addr, monitor_mask;
> > +	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;
> 
> Does this mean monitor_mask will also change to RTE_ATOMIC(uint32_t)? 
> 
> Seems not necessary.

looks like a mistake, i will review and make clear in next revision.

thanks for spotting it.

> 
> >  	unsigned int trigger_index;
> > 
> >  	trigger_index = monitor_id / HV_MON_TRIG_LEN;
> >  	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
> > 
> > -	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
> > +	monitor_addr =
> > +	    (uint32_t __rte_atomic
> > +*)&channel->monitor_page->trigs[trigger_index].pending;
> >  	vmbus_sync_set_bit(monitor_addr, monitor_mask);  }
> > 
> > --
> > 1.8.3.1
diff mbox series

Patch

diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
index 4d74df3..e96a4eb 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -19,22 +19,23 @@ 
 #include "private.h"
 
 static inline void
-vmbus_sync_set_bit(volatile uint32_t *addr, uint32_t mask)
+vmbus_sync_set_bit(volatile RTE_ATOMIC(uint32_t) *addr, uint32_t mask)
 {
 	/* Use GCC builtin which atomic does atomic OR operation */
-	__atomic_fetch_or(addr, mask, __ATOMIC_SEQ_CST);
+	rte_atomic_fetch_or_explicit(addr, mask, rte_memory_order_seq_cst);
 }
 
 static inline void
 vmbus_set_monitor(const struct vmbus_channel *channel, uint32_t monitor_id)
 {
-	uint32_t *monitor_addr, monitor_mask;
+	RTE_ATOMIC(uint32_t) *monitor_addr, monitor_mask;
 	unsigned int trigger_index;
 
 	trigger_index = monitor_id / HV_MON_TRIG_LEN;
 	monitor_mask = 1u << (monitor_id % HV_MON_TRIG_LEN);
 
-	monitor_addr = &channel->monitor_page->trigs[trigger_index].pending;
+	monitor_addr =
+	    (uint32_t __rte_atomic *)&channel->monitor_page->trigs[trigger_index].pending;
 	vmbus_sync_set_bit(monitor_addr, monitor_mask);
 }