Re: [Minios-devel] [UNIKRAFT PATCH v4 09/16] include/uk: bitopts.h - remove already existing functions

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi,

On 09/06/2018 03:49 PM, Yuri Volchkov wrote:
> diff --git a/include/uk/bitops.h b/include/uk/bitops.h
> index e003a43..6095004 100644
> --- a/include/uk/bitops.h
> +++ b/include/uk/bitops.h
> @@ -58,11 +58,11 @@
> -#define        __set_bit(i, a)                                                 
> \
> -       atomic_set_long(&((volatile unsigned long *)(a))[BIT_WORD(i)], 
> BIT_MASK(i))
> -
> -#define        set_bit(i, a)                                                   
> \
> -       atomic_set_long(&((volatile unsigned long *)(a))[BIT_WORD(i)], 
> BIT_MASK(i))
> -
> -#define        __clear_bit(i, a)                                               
> \
> -       atomic_clear_long(&((volatile unsigned long *)(a))[BIT_WORD(i)], 
> BIT_MASK(i))
> -
> -#define        clear_bit(i, a)                                                 
> \
> -       atomic_clear_long(&((volatile unsigned long *)(a))[BIT_WORD(i)], 
> BIT_MASK(i))
> -
> -#define        test_bit(i, a)                                                  
> \
> -       !!(READ_ONCE(((volatile unsigned long *)(a))[BIT_WORD(i)]) & 
> BIT_MASK(i))
> +#define __set_bit(i, a)        ukarch_set_bit(i, a)
> +#define set_bit(i, a)          ukarch_set_bit_sync(i, a)
> +#define __clear_bit(i, a)      ukarch_clr_bit(i, a)
> +#define clear_bit(i, a)        ukarch_clr_bit_sync(i, a)
> +#define test_bit(i, a)         ukarch_test_bit(i, a)
Two things about this. First the easy one :)
The FreeBSD code treated the two versions (with and without double 
underscores) exactly the same, while we're using sync and non-sync 
versions. That is probably not a bad choice (and seems to mirror what, 
for example, the Linux kernel does), but I think this decision should be 
reflected in the commit message.
Now the more complicated one: test_bit. The FreeBSD version uses a 
READ_ONCE, while we don't have anything like that in our test_bit. On 
the first look, dropping that without replacement looks dangerous. Then 
again, it also operates on a volatile, so... I'm not sure what exactly 
the READ_ONCE is doing here on top of that? The FreeBSD commit that 
introduced this (f48e4f4e39) is also not so helpful with its commit message.
Also, would test_bit have to be atomic, and would our implementation be? 
Could we use __atomic_load with the right __ATOMIC memorder to make our 
test_bit perform as expected?
I'm anxious to get this right the first time around, because it sounds 
like a horrible problem to debug if we don't.
Cheers,
Florian

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel