Re: [PATCH 3/5] fs: sys_ringbuffer

[Thomas Gleixner <tglx@linutronix.de>]

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