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

[Anuj Gupta <anuj20.g@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On Mon, Nov 25, 2024 at 02:58:19PM +0000, Pavel Begunkov wrote:
> On 11/25/24 07:06, Anuj Gupta wrote:
> 
> ATTR_TYPE sounds too generic, too easy to get a symbol collision
> including with user space code.
> 
> Some options: IORING_ATTR_TYPE_PI, IORING_RW_ATTR_TYPE_PI.
> If it's not supposed to be io_uring specific can be
> IO_RW_ATTR_TYPE_PI
> 

Sure, will change to a different name in the next iteration.

> > +
> > +/* attribute information along with type */
> > +struct io_uring_attr {
> > +	enum io_uring_attr_type	attr_type;
> 
> I'm not against it, but adding a type field to each attribute is not
> strictly needed, you can already derive where each attr placed purely
> from the mask. Are there some upsides? But again I'm not against it.
> 

The mask indicates what all attributes have been passed. But while
processing we would need to know where exactly the attributes have been
placed, as attributes are of different sizes (each attribute is of
fixed size though) and they could be placed in any order. Processing when
multiple attributes are passed would look something like this:

attr_ptr = READ_ONCE(sqe->attr_ptr);
attr_mask = READ_ONCE(sqe->attr_type_mask);
size = total_size_of_attributes_passed_from_attr_mask;

copy_from_user(attr_buf, attr_ptr, size);

while (size > 0) {
	if (sizeof(io_uring_attr_type) > size)
		break;

	attr_type = get_type(attr_buf);
	attr_size = get_size(attr_type);

	process_attr(attr_type, attr_buf);
	attr_buf += attr_size;
	size -= attr_size;
}

We cannot derive where exactly the attribute information is placed
purely from the mask, so we need the type field. Do you see it
differently?

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]