Re: [PATCH for-next] RDMA/cma: Replace RMW with atomic bit-ops

[Haakon Bugge <haakon.bugge@oracle.com>]

> On 16 Jun 2021, at 17:02, Stefan Metzmacher <metze@samba.org> wrote:
> 
> Hi Håkon,
> 
>> +#define BIT_ACCESS_FUNCTIONS(b)							\
>> +	static inline void set_##b(unsigned long flags)				\
>> +	{									\
>> +		/* set_bit() does not imply a memory barrier */			\
>> +		smp_mb__before_atomic();					\
>> +		set_bit(b, &flags);						\
>> +		/* set_bit() does not imply a memory barrier */			\
>> +		smp_mb__after_atomic();						\
>> +	}									\
> 
> Don't you need to pass flags by reference to the inline function?
> As far as I can see set_bit() operates on a temporary variable.

Good catch Stefan! It started off as a define, then it worked, but as you say, this doesn't work when we pass flags by value to the inline function. Bummer!

I'll send a v2 tomorrow and see if I can include other review comments as well.

Again, thanks for the review !


Håkon

> 
> In order to check if inline functions are special I wrote this little check:
> 
> $ cat inline_arg.c
> #include <stdio.h>
> 
> static inline void func(unsigned s, unsigned *p)
> {
>        printf("&s=%p p=%p\n", &s, p);
> }
> 
> int main(void)
> {
>        unsigned flags = 0;
> 
>        func(flags, &flags);
>        return 0;
> }
> 
> $ gcc -O0 -o inline_arg inline_arg.c
> $ ./inline_arg
> &s=0x7ffd3a71616c p=0x7ffd3a716184
> 
> $ gcc -O6 -o inline_arg inline_arg.c
> $ ./inline_arg
> &s=0x7ffd87781064 p=0x7ffd87781060
> 
> It means s doesn't magically become 'flags' of the caller...
> 
> I'm I missing something?
> 
> metze