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
