On Tue, Jul 26, 2022 at 07:47:21PM +0200, Christoph Hellwig wrote: > On Tue, Jul 26, 2022 at 10:21:04PM +0800, Ming Lei wrote: > > But most of fields in ublk_params aren't required for one device, > > sending them all isn't friendly from both userspace and kernel side. > > I think it actually is easier for both. For the kernel the benefit > is pretty clear seen by this patch, and for userspae there also is > a lot less boilerplate code. If everything is put into single structure: 1) it is fragile for both userspace/driver side making/paring code - such as, how to parse zeroed fields? With grouping parameters, if one type parameter isn't sent via set param command, the userspace just meant we do not care this parameter just like one real hardare, driver feels free to use the default. But if it is sent as zero, driver may have to parse it as zero and use zero, sometimes it can be wrong, and parsing parameter with zero for kernel becomes hard. With grouping parameter, if one parameter is sent from userspace, ublk driver just parses & uses it explicitly. There isn't any burden for driver for making such decision. 2) ublk driver has to parse every fields, and the code could be harder to organize, since there isn't parameter group. With grouping parameters, we just need to implement two callbacks when adding new parameter type. So code is organized pretty well. Wrt. cleanness, I will send V2 for review, and please feel free to let me know where isn't clean, and I am happy to make it correct/clean from the beginning. And userspace change will be updated too, and we can see if userspace side has much boilerplate code. > > > Especially inside kernel, a big chunk buffer is allocated for > > the structure, but only few fields are useful for one device. There can > > be lots of ublk device, and total wasted memory can't be ignored. > > I think even right now the memory usage is less. If a lot (and I mean Adding segment limits & zoned limits are in my todo list. > a lot) new paramters are added, it might be slightly higher than the > minimal config with an xarray, but not to the point where it matters. Alibaba said their case needs to run lots of ublk devices in one machine. It is ABI interface, and I'd rather make it easy extending from the beginning given there is the requirement for running lots of devices. > > > If we group parameters, it is easier to extend by adding new parameter > > type. One ublk device usually just uses several parameter types. > > And the xarray implementation is simple enough too, which is just one > > array, but really sparsed. > > Well, it isn't exactly simple. And the apprach of a struct that just > grows additional members has worked perfectly fine all over the kernel. In your patch, there isn't length field in ublk_params, not sure ublk driver can copy back correct parameter. If userspace is old, on new kernel with bigger structure size userspace buffer could be corrupted by copying back kernel's structure. So here one length field is really a must. With grouping parameters, each type has fixed length which can't be changed since it is added, we can verify it easily in both set/get param command, meantime the parameter type can be verified reliably in driver side, so grouping parameter is more reliable. Thanks, Ming
