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

[Long Li <longli@microsoft.com>]

> 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