Conversation
For all functions which follow the pattern where we first call an ioctl() to know the pointer sizes, allocate Vec, then call it again to fill the actual data we passed, we can actually allocate the struct only once and fill only the pointers after the first ioctl(). This reduces the stack usage slightly, and avoids having to copy data that doesn’t need to be copied (usually the sizes).
| unique_len: sizes.unique_len, | ||
| unique: map_ptr!(&buf), | ||
| }; | ||
| busid.unique = map_ptr!(&buf); |
There was a problem hiding this comment.
Hmm I am slightly worried, that this would mean, we way end up setting some field, we didn't want to use.
This would also reuse the allocation:
*busid = drm_unique {
unique_len: busid.unique_len,
unique: map_ptr!(&buf),
}
While relatively useless in this particular case, it would reduce changes of this patch and keep the current behavior including filling the rest of the fields with ..Default::default in many cases. Would you be willing to re-work the patch to do that?
There was a problem hiding this comment.
We already set the rest of the fields to their Default::default() value on line 62, when allocating the entire data. Or is it that you don’t trust the kernel not to modify some fields in the first call? I would find that extremely unlikely, and we kind of have to trust the kernel to obey its uAPI.
And the unique_len field in this case is the one being set by the kernel during the first ioctl().
There was a problem hiding this comment.
It's more that I don't trust us to always reset any fields that we would potentially not set on the second request. I don't think we have a case like this atm.
There was a problem hiding this comment.
There should be no need for that, as this uAPI is explicitly designed for the usage pattern I’m suggesting. You pass the struct a first time, it gives you the lengths of the buffers you have to allocate, you allocate them, put them in the struct, then pass the same struct a second time.
New DRM ioctls will always follow this pattern, as do many other uAPIs in the kernel.
For all functions which follow the pattern where we first call an
ioctl()to know the pointer sizes, allocateVec, then call it again to fill the actual data we passed, we can actually allocate the struct only once and fill only the pointers after the firstioctl().This reduces the stack usage slightly, and avoids having to copy data that doesn’t need to be copied (usually the sizes).