RE: [EXTERNAL] Re: [Patch v3 6/6] bus/vmbus: set event for channel without monitoring support

[Long Li <longli@microsoft.com>]

> Subject: RE: [EXTERNAL] Re: [Patch v3 6/6] bus/vmbus: set event for channel
> without monitoring support
> 
> > Subject: [EXTERNAL] Re: [Patch v3 6/6] bus/vmbus: set event for
> > channel without monitoring support
> >
> > On Fri,  4 Apr 2025 17:35:38 -0700
> > longli@linuxonhyperv.com wrote:
> >
> > > diff --git a/drivers/bus/vmbus/vmbus_channel.c
> > > b/drivers/bus/vmbus/vmbus_channel.c
> > > index bccef168d3..81e8096190 100644
> > > --- a/drivers/bus/vmbus/vmbus_channel.c
> > > +++ b/drivers/bus/vmbus/vmbus_channel.c
> > > @@ -24,6 +24,19 @@ vmbus_sync_set_bit(volatile RTE_ATOMIC(uint32_t)
> > *addr, uint32_t mask)
> > >  	rte_atomic_fetch_or_explicit(addr, mask,
> > > rte_memory_order_seq_cst); }
> > >
> > > +static inline void
> > > +vmbus_send_interrupt(const struct rte_vmbus_device *dev, uint32_t
> > > +relid) {
> > > +	RTE_ATOMIC(uint32_t) *int_addr;
> > > +	uint32_t int_mask;
> > > +
> > > +	int_addr = (RTE_ATOMIC(uint32_t)*) (dev->int_page + relid / 32);
> > > +	int_mask = 1u << (relid % 32);
> > > +	vmbus_sync_set_bit(int_addr, int_mask);
> > > +
> > > +	vmbus_uio_irq_control(dev, 1);
> > > +}
> > > +
> >
> > This part doesn't look right. RTE_ATOMIC() is just a macro to add the
> > _Atomic attribute.
> >
> > Can it be simplified like this?
> >
> >
> > static inline void
> > vmbus_sync_set_bit(RTE_ATOMIC(uint32_t *) addr, uint32_t mask) {
> > 	rte_atomic_fetch_or_explicit(addr, mask, rte_memory_order_seq_cst); }
> >
> > static inline void
> > vmbus_send_interrupt(const struct rte_vmbus_device *dev, uint32_t relid) {
> > 	RTE_ATOMIC(uint32_t *) int_addr;
> > 	uint32_t int_mask;
> >
> > 	int_addr = dev->int_page + relid / 32;
> > 	int_mask = 1u << (relid % 32);
> > 	vmbus_sync_set_bit(int_addr, int_mask);
> >
> > 	vmbus_uio_irq_control(dev, 1);
> > }
> 
> Hi Stephen,
> 
> I need to go back to this version of the patch, as v5 doesn't compile successfully
> under clang with the following error:
> 
> [459/3553] Compiling C object drivers/libtmp_rte_bus_vmbus.a.p/bus_vmbus
> ccache gcc -Idrivers/libtmp_rte_bus_vmbus.a.p -Idrivers -I../drivers -
> Idrivers/bus/vmbus -I../drivers/bus/vmbus -I../drivers/bus/vmbus/linux -
> Ilib/eal/common -I../lib/eal/common -I. -I.. -Iconfig -I../config -Ilib/eal/include -
> I../lib/eal/include -Ilib/eal/linux/include -I../lib/eal/linux/include -
> Ilib/eal/x86/include -I../lib/eal/x86/include -I../kernel/linux -Ilib/eal -I../lib/eal -
> Ilib/kvargs -I../lib/kvargs -Ilib/log -I../lib/log -Ilib/metrics -I../lib/metrics -
> Ilib/telemetry -I../lib/telemetry -fdiagnostics-color=always -pipe -
> D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O2 -g -
> include rte_config.h -Wvla -Wcast-qual -Wdeprecated -Wformat -Wformat-
> nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes -
> Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare -
> Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned -Wno-
> missing-field-initializers -D_GNU_SOURCE -fPIC -march=corei7 -mrtm -
> DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
> -Wno-address-of-packed-member -DRTE_LOG_DEFAULT_LOGTYPE=bus.vmbus -
> MD -MQ drivers/libtmp_rte_bus_vmbus.a.p/bus_vmbus_vmbus_channel.c.o -MF
> drivers/libtmp_rte_bus_vmbus.a.p/bus_vmbus_vmbus_channel.c.o.d -o
> drivers/libtmp_rte_bus_vmbus.a.p/bus_vmbus_vmbus_channel.c.o -
> c ../drivers/bus/vmbus/vmbus_channel.c
> ../drivers/bus/vmbus/vmbus_channel.c: In function 'vmbus_set_monitor':
> ../drivers/bus/vmbus/vmbus_channel.c:51:22: error: assignment to 'uint32_t *'
> {aka 'unsigned int *'} from incompatible pointer type '_Atomic uint32_t *' {aka
> '_Atomic unsigned int *'} [-Werror=incompatible-pointer-types]
>    51 |         monitor_addr = &channel->monitor_page-
> >trigs[trigger_index].pending;
>       |                      ^
> cc1: all warnings being treated as errors
> 
> I think it's okay to define those pointers as "RTE_ATOMIC(uint32_t) *". The
> pointers themselves are not atomic, but data in the addresses they point to are
> atomic.
> 
> Do you think it is okay to proceed with this patch?
> 
> Thanks,
> Long

I sent out v6 reverting the patch to this version. This compiles okay with clang and stdatomic flags.

Thanks,
Long