On Thu, Nov 7, 2024 at 11:25 AM Christoph Hellwig <hch@xxxxxx> wrote:
>
> > +enum io_uring_sqe_ext_cap_bits {
> > + EXT_CAP_PI_BIT,
> > + /*
> > + * not a real extended capability; just to make sure that we don't
> > + * overflow
> > + */
> > + EXT_CAP_LAST_BIT,
> > +};
> > +
> > +/* extended capability flags */
> > +#define EXT_CAP_PI (1U << EXT_CAP_PI_BIT)
>
> This is getting into nitpicking, but is the a good reason to have that
> enum, which is never used as a type and the values or only defined to
> actually define the bit positions below? That's a bit confusing to
> me.
The enum is added to keep a check on the number of flags that can
be added, and make sure that we don't overflow.
>
> Also please document the ABI for EXT_CAP_PI, right now this is again
> entirely undocumented.
>
We are planning to document this in man/io_uring_enter.2 in the liburing
repo, right after this series goes in. Or should it go somewhere else?
> > +/* Second half of SQE128 for IORING_OP_READ/WRITE */
> > +struct io_uring_sqe_ext {
> > + __u64 rsvd0[4];
> > + /* if sqe->ext_cap is EXT_CAP_PI, last 32 bytes are for PI */
> > + union {
> > + __u64 rsvd1[4];
> > + struct {
> > + __u16 flags;
> > + __u16 app_tag;
> > + __u32 len;
> > + __u64 addr;
> > + __u64 seed;
> > + __u64 rsvd;
> > + } rw_pi;
> > + };
>
> And this is not what I though we discussed before. By having a
> union here you imply some kind of "type" again that is switched
> on a value, and not flags indication the presence of potential
> multiple optional and combinable features. This is what I would
> have expected here based on the previous discussion:
The attempt here is that if two extended capabilities are not known to
co-exist then they can be kept in the same place. Since each extended
capability is now a flag, we can check what combinations are valid and
throw an error in case of incompatibility. Do you see this differently?
>
> struct io_uring_sqe_ext {
> /*
> * Reservered for please tell me what and why it is in the beginning
> * and not the end:
> */
> __u64 rsvd0[4];
This space is reserved for extended capabilities that might be added down
the line. It was at the end in the earlier versions, but it is moved
to the beginning
now to maintain contiguity with the free space (18b) available in the first SQE,
based on previous discussions [1].
[1] https://lore.kernel.org/linux-block/ceb58d97-b2e3-4d36-898d-753ba69476be@xxxxxxxxxxx/
>
> /*
> * Only valid when EXT_CAP_PI is set:
> */
> __u16 pi_flags; /* or make this generic flags, dunno? */
> __u16 app_tag;
> __u32 pi_len;
> __u64 pi_addr;
> __u64 pi_seed;
>
> __u64 rsvd1;
> };
>
