Re: [PATCH v10 06/10] io_uring: introduce attributes for read/write and PI support

[Pavel Begunkov <asml.silence@gmail.com>]

On 11/26/24 13:54, Anuj Gupta wrote:
> On Tue, Nov 26, 2024 at 01:01:03PM +0000, Pavel Begunkov wrote:
>> On 11/25/24 07:06, Anuj Gupta wrote:
>> ...
>>> +	/* type specific struct here */
>>> +	struct io_uring_attr_pi	pi;
>>> +};
>>
>> This also looks PI specific but with a generic name. Or are
>> attribute structures are supposed to be unionised?
> 
> Yes, attribute structures would be unionised here. This is done so that
> "attr_type" always remains at the top. When there are multiple attributes
> this structure would look something like this:
> 
> /* attribute information along with type */
> struct io_uring_attr {
> 	enum io_uring_attr_type attr_type;
> 	/* type specific struct here */
> 	union {
> 		struct io_uring_attr_pi	pi;
> 		struct io_uring_attr_x	x;
> 		struct io_uring_attr_y	y;
> 	};
> };
> 
> And then on the application side for sending attribute x, one would do:
> 
> io_uring_attr attr;
> attr.type = TYPE_X;
> prepare_attr(&attr.x);

Hmm, I have doubts it's going to work well because the union
members have different sizes. Adding a new type could grow
struct io_uring_attr, which is already bad for uapi. And it
can't be stacked:

io_uring_attr attrs[2] = {..., ...}
sqe->attr_ptr = &attrs;
...

This example would be incorrect. Even if it's just one attribute
the user would be wasting space on stack. The only use for it I
see is having ephemeral pointers during parsing, ala

void parse(voud *attributes, offset) {
	struct io_uring_attr *attr = attributes + offset;
	
	if (attr->type == PI) {
		process_pi(&attr->pi);
		// or potentially fill_pi() in userspace
	}
}

But I don't think it's worth it. I'd say, if you're leaving
the structure, let's rename it to struct io_uring_attr_type_pi
or something similar. We can always add a new one later, it
doesn't change the ABI.

-- 
Pavel Begunkov