Re: [PATCH 3/5] fs: sys_ringbuffer

Thomas Gleixner
2024-06-23T23:16:17Z

Kent!

On Sun, Jun 23 2024 at 18:21, Kent Overstreet wrote:
> On Mon, Jun 24, 2024 at 12:13:36AM +0200, Thomas Gleixner wrote:
>> > + /*
>> > + * We use u32s because this type is shared between the kernel and
>> > + * userspace - ulong/size_t won't work here, we might be 32bit userland
>> > + * and 64 bit kernel, and u64 would be preferable (reduced probability
>> > + * of ABA) but not all architectures can atomically read/write to a u64;
>> > + * we need to avoid torn reads/writes.
>>
>> union rbmagic {
>> u64 __val64;
>> struct {
>> // TOOTIRED: Add big/little endian voodoo
>> u32 __val32;
>> u32 __unused;
>> };
>> };
>>
>> Plus a bunch of accessors which depend on BITS_PER_LONG, no?
>
> Not sure I follow?
>
> I know biendian machines exist, but I've never heard of both big and
> little endian being used at the same time. Nor why we'd care about
> BITS_PER_LONG? This just uses fixed size integer types.

Read your comment above. Ideally you want to use u64, right?

The problem is that you can't do this unconditionally because of 32-bit
systems which do not support 64-bit atomics.

So a binary which is compiled for 32-bit might unconditionally want the
32-bit accessors. Ditto for 32-bit kernels.

The 64bit kernel where it runs on wants to utilize u64, right?

That's fortunately a unidirectional problem as 64-bit user space cannot
run on a 32-bit kernel ever.

struct ringbuffer_ctrl {
union rbmagic head;
...
};

#ifdef __BITS_PER_LONG == 64
static __always_inline u64 read_head(struct ringbuffer_ctrl *rb)
{
return rb->head.__val64;
}

static __always_inline void write_head(struct ringbuffer_ctrl *rb, u64 val)
{
rb->head.__val64 = val;
}
#else
static __always_inline u64 read_head(struct ringbuffer_ctrl *rb)
{
return rb->head.__val32;
}

static __always_inline void write_head(struct ringbuffer_ctrl *rb, u64 val)
{
rb->head.__val32 = (u32)val;
}
#endif

A 64-bit kernel uses u64 while a 32-bit kernel uses u32. Same for user
space.

The ABA concern for 32-bit does not go away, but for 64-bit userspace
you get what you want, no?

Now why do you have to care about endianess?

union rbmagic {
u64 __val64;
struct {
u32 __val32;
u32 __unused;
};
};

works only correctly for LE. But it does not work for BE because BE
obviously requires the u32 members to be in reverse order:

union rbmagic {
u64 __val64;
struct {
u32 __unused;
u32 __val32;
};
};

That's a compile time decision. You can't run a BE binary on a LE kernel
or the other way around.

So they have to agree on the endianess, but BE has the reverse byte
order. That's why you need to have another #ifdef there.

Thanks,

tglx