Skip to content

drm-ffi: Allocate stack structs only once - #240

Open
linkmauve wants to merge 1 commit into
Smithay:developfrom
linkmauve:less-stack
Open

linkmauve wants to merge 1 commit into
Smithay:developfrom
linkmauve:less-stack

Conversation

@linkmauve

Copy link
Copy Markdown
Contributor

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).

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).
Comment thread drm-ffi/src/lib.rs
unique_len: sizes.unique_len,
unique: map_ptr!(&buf),
};
busid.unique = map_ptr!(&buf);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@linkmauve linkmauve Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants