Re: [Xen-devel] [resend PATCH] xen: common: rbtree: ported updates from linux tree

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.05.17 at 19:21, <kpraveen.lkml@xxxxxxxxx> wrote:
> The patch contains the updated version of rbtree implementation from linux
> kernel tree containing the fixes so far handled.

I suppose this isn't just fixes, but also enhancements. Furthermore
I'd appreciate if you recorded the Linux version this was taken from,
so that anyone wanting to do another upgrade would know what
the baseline is. In any event, as long as this is just a general
overhaul and upgrade, I'd like to either see individual bugs pointed
out which get fixed _and_ which affect us, or I'd expect this to be
part of a series which actually requires some of the new functionality.
Otherwise it is e.g. hard to understand why ...

> Signed-off-by: Praveen Kumar <kpraveen.lkml@xxxxxxxxx>
> ---
>  xen/common/rbtree.c                | 748 
> +++++++++++++++++++++++++------------
>  xen/include/xen/compiler.h         |  60 +++
>  xen/include/xen/rbtree.h           | 120 ++++--
>  xen/include/xen/rbtree_augmented.h | 283 ++++++++++++++

... namely this last (new) header (and what it provides) is needed
at all.

I don't think there's much point in reviewing these changes if
indeed they've been taken from Linux literally (I'm sure you
would have pointed out meaningful changes in the commit
message).

> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -127,4 +127,64 @@
>  # define CLANG_DISABLE_WARN_GCC_COMPAT_END
>  #endif
>  
> +#include <xen/types.h>

Anything requiring this header very unlikely belongs into compiler.h.

> +#ifndef __always_inline
> +#define __always_inline inline
> +#endif

Please use always_inline instead, which we already have.

> +#define __READ_ONCE_SIZE                                       \
> +({                                                             \
> +    switch(size) {                                             \
> +    case 1: *(__u8 *)res = *(volatile __u8 *)p; break;         \
> +    case 2: *(__u16 *)res = *(volatile __u16 *)p; break;       \
> +    case 4: *(__u32 *)res = *(volatile __u32 *)p; break;       \
> +    case 8: *(__u64 *)res = *(volatile __u64 *)p; break;       \

No new uses of __u<n> or u<n> or their signed counterparts
please. We have {,u}int<n>_t for that purpose. Iirc even Linux
maintainers nowadays object to these double underscore prefixed
names outside of user visible header files.

> +    default:                                                   \
> +        barrier();                                             \
> +        __builtin_memcpy((void *)res, (const void *)p, size);  \
> +        barrier();                                             \

Compiler barriers aren't equivalents of uses of "volatile" on all
architectures, so the correctness here would need to be
explained.

> +    }                                                          \
> +})
> +
> +static __always_inline
> +void __read_once_size(const volatile void *p, void *res, int size)
> +{
> +    __READ_ONCE_SIZE;
> +}
> +
> +static __always_inline
> +void __write_once_size(volatile void *p, void *res, int size)
> +{
> +    switch (size) {
> +    case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
> +    case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
> +    case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
> +    case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
> +    default:
> +        barrier();
> +        __builtin_memcpy((void *)p, (const void *)res, size);
> +        barrier();
> +    }
> +}
> +
> +#define __READ_ONCE(x, check)                                  \
> +({                                                             \
> +    union { typeof(x) __val; char __c[1]; } __u;               \
> +    __read_once_size(&(x), __u.__c, sizeof(x));                \
> +})

The "check" parameter is unused, so ...

> +#define READ_ONCE(x) __READ_ONCE(x, 1)

... this wrapper can be dropped.

> +#define WRITE_ONCE(x, val)                                     \
> +({                                                             \
> +    union { typeof(x) __val; char __c[1]; } __u =              \
> +        { .__val = (__force typeof(x)) (val) };                \
> +    __write_once_size(&(x), __u.__c, sizeof(x));               \
> +    __u.__val;                                                 \
> +})
> +
> +
> +
> +
>  #endif /* __LINUX_COMPILER_H */

No way you get to add four blank lines in a row to any file.

In any event it is not clear to me whether we really need all of this:
Are there properties at the use sites which neither
{read,write}_atomic() nor ACCESS_ONCE() can fulfill? And if indeed
we need yet another flavor, you'd need to do away with all these
underscore prefixed names.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel