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 10:40, Anuj Gupta wrote:
> 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?

In the mask version I outlined attributes go in the array in order
of their types, max 1 attribute of each type, in which case the
mask fully describes the placement.

static attr_param_sizes[] = {[TYPE_PI] = sizeof(pi), ...};
mask = READ_ONCE(sqe->mask);
off = 0;

for (type = 0; type < NR_TYPE; type++) { // or find_next_bit trick
	if (!(mask & BIT(i)))
		continue;
	process(type=i, pointer= base + attr_param_sizes[i]);
	off += attr_param_sizes[i];
}


Maybe it's a good idea to have a type field for double checking
though, and with it we don't have to commit to one version or
another yet.

-- 
Pavel Begunkov