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

[Anuj gupta <anuj1072538@gmail.com>]

On Tue, Nov 26, 2024 at 9:14 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> 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.
>

In that case I can just drop the io_uring_attr_pi structure then. We can
keep the mask version where we won't need the type and attributes would go
in the array in order of their types as you suggested here [1]. Does that
sound fine?

[1] https://lore.kernel.org/io-uring/37ba07f6-27a5-45bc-86c4-df9c63908ef9@gmail.com/