[v2,38/45] bus/vmbus: use rte stdatomic API
Checks
Commit Message
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
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);
> }
>
> > 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
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
> > 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.
> 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
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
@@ -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);
}