Alexandre Courbot
2025-Nov-26 01:16 UTC
[PATCH v2 1/3] rust: helpers: Add list helpers for C linked list operations
On Wed Nov 26, 2025 at 3:16 AM JST, Joel Fernandes wrote:> > >> On Nov 25, 2025, at 9:52?AM, Alexandre Courbot <acourbot at nvidia.com> wrote: >> >> ?On Wed Nov 12, 2025 at 2:13 AM JST, Joel Fernandes wrote: >>> Add Rust helper functions for common C linked list operations >>> that are implemented as macros or inline functions and thus not >>> directly accessible from Rust. >>> >>> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> >>> --- >>> rust/helpers/helpers.c | 1 + >>> rust/helpers/list.c | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 33 insertions(+) >>> create mode 100644 rust/helpers/list.c >>> >>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c >>> index 79c72762ad9c..634fa2386bbb 100644 >>> --- a/rust/helpers/helpers.c >>> +++ b/rust/helpers/helpers.c >>> @@ -32,6 +32,7 @@ >>> #include "io.c" >>> #include "jump_label.c" >>> #include "kunit.c" >>> +#include "list.c" >>> #include "maple_tree.c" >>> #include "mm.c" >>> #include "mutex.c" >>> diff --git a/rust/helpers/list.c b/rust/helpers/list.c >>> new file mode 100644 >>> index 000000000000..fea2a18621da >>> --- /dev/null >>> +++ b/rust/helpers/list.c >>> @@ -0,0 +1,32 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +/* >>> + * Helpers for C Circular doubly linked list implementation. >>> + */ >>> + >>> +#include <linux/list.h> >>> + >>> +bool rust_helper_list_empty(const struct list_head *head) >>> +{ >>> + return list_empty(head); >>> +} >>> + >>> +void rust_helper_list_del(struct list_head *entry) >>> +{ >>> + list_del(entry); >>> +} >>> + >>> +void rust_helper_INIT_LIST_HEAD(struct list_head *list) >>> +{ >>> + INIT_LIST_HEAD(list); >>> +} >>> + >>> +void rust_helper_list_add(struct list_head *new, struct list_head *head) >>> +{ >>> + list_add(new, head); >>> +} >>> + >>> +void rust_helper_list_add_tail(struct list_head *new, struct list_head *head) >>> +{ >>> + list_add_tail(new, head); >>> +} >> >> Just noticed, but of these helpers only `INIT_LIST_HEAD` and >> `list_add_tail` seem to be used (in doccomments). > > Correct, but it makes sense to add the most obvious/common ones (also to make it clear that using these are supported)."It makes sense" is subjective, and in this case I am confident it is not the right intuition to add dead code just for the sake of it. Each of these helpers adds a potential breakage point from the C API should the latter change, so we should only add them if they are indeed necessary. Actually, some of these helpers are not used when they could have been - you have a `is_empty` method that rewrites the C function instead of calling the helper. The only helpers that are unjustified as of now as `list_add` and `list_del`, and these are easy to add when they become necessary. But this raises an interesting dilemma: these helpers cannot be inlined and add the overhead of a function call. On the other hand, the definition of `list_head` is so excessively simple that manipulating it directly is virtually as intuitive as invoking the helper - and doesn't bear the overhead. So should we double-down on these helpers, or just drop them completely and re-implement the list functionality we need for increased performance?
John Hubbard
2025-Nov-26 01:39 UTC
[PATCH v2 1/3] rust: helpers: Add list helpers for C linked list operations
On 11/25/25 5:16 PM, Alexandre Courbot wrote:> On Wed Nov 26, 2025 at 3:16 AM JST, Joel Fernandes wrote: >>> On Nov 25, 2025, at 9:52?AM, Alexandre Courbot <acourbot at nvidia.com> wrote: >>> ?On Wed Nov 12, 2025 at 2:13 AM JST, Joel Fernandes wrote:...>> Correct, but it makes sense to add the most obvious/common ones (also to make it clear that using these are supported). > > "It makes sense" is subjective, and in this case I am confident it is > not the right intuition to add dead code just for the sake of it.Yes. I am increasingly uneasy with the Rust for Linux approach, and now the Nova approach, of adding in "things we might need". The kernel, for example linux-mm, is usually quite fiercely pushing for "prove you need this, by adding in the calling code as part of your series". Drifting away from this makes it hard to know how the calling code *actually* looks.> > Each of these helpers adds a potential breakage point from the C API > should the latter change, so we should only add them if they are indeed > necessary. >Yes please! Just add what you need. Other goodies can come later.> Actually, some of these helpers are not used when they could have been - > you have a `is_empty` method that rewrites the C function instead of > calling the helper. The only helpers that are unjustified as of now as > `list_add` and `list_del`, and these are easy to add when they become > necessary. > > But this raises an interesting dilemma: these helpers cannot be inlined > and add the overhead of a function call. On the other hand, the > definition of `list_head` is so excessively simple that manipulating it > directly is virtually as intuitive as invoking the helper - and doesn't > bear the overhead. So should we double-down on these helpers, or just > drop them completely and re-implement the list functionality we need for > increased performance?Do the "it runs faster" thing. :) thanks, -- John Hubbard
Joel Fernandes
2025-Nov-26 18:50 UTC
[PATCH v2 1/3] rust: helpers: Add list helpers for C linked list operations
> On Nov 25, 2025, at 8:16?PM, Alexandre Courbot <acourbot at nvidia.com> wrote: > > ?On Wed Nov 26, 2025 at 3:16 AM JST, Joel Fernandes wrote: >> >> >>>> On Nov 25, 2025, at 9:52?AM, Alexandre Courbot <acourbot at nvidia.com> wrote: >>> >>> ?On Wed Nov 12, 2025 at 2:13 AM JST, Joel Fernandes wrote: >>>> Add Rust helper functions for common C linked list operations >>>> that are implemented as macros or inline functions and thus not >>>> directly accessible from Rust. >>>> >>>> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> >>>> --- >>>> rust/helpers/helpers.c | 1 + >>>> rust/helpers/list.c | 32 ++++++++++++++++++++++++++++++++ >>>> 2 files changed, 33 insertions(+) >>>> create mode 100644 rust/helpers/list.c >>>> >>>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c >>>> index 79c72762ad9c..634fa2386bbb 100644 >>>> --- a/rust/helpers/helpers.c >>>> +++ b/rust/helpers/helpers.c >>>> @@ -32,6 +32,7 @@ >>>> #include "io.c" >>>> #include "jump_label.c" >>>> #include "kunit.c" >>>> +#include "list.c" >>>> #include "maple_tree.c" >>>> #include "mm.c" >>>> #include "mutex.c" >>>> diff --git a/rust/helpers/list.c b/rust/helpers/list.c >>>> new file mode 100644 >>>> index 000000000000..fea2a18621da >>>> --- /dev/null >>>> +++ b/rust/helpers/list.c >>>> @@ -0,0 +1,32 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> + >>>> +/* >>>> + * Helpers for C Circular doubly linked list implementation. >>>> + */ >>>> + >>>> +#include <linux/list.h> >>>> + >>>> +bool rust_helper_list_empty(const struct list_head *head) >>>> +{ >>>> + return list_empty(head); >>>> +} >>>> + >>>> +void rust_helper_list_del(struct list_head *entry) >>>> +{ >>>> + list_del(entry); >>>> +} >>>> + >>>> +void rust_helper_INIT_LIST_HEAD(struct list_head *list) >>>> +{ >>>> + INIT_LIST_HEAD(list); >>>> +} >>>> + >>>> +void rust_helper_list_add(struct list_head *new, struct list_head *head) >>>> +{ >>>> + list_add(new, head); >>>> +} >>>> + >>>> +void rust_helper_list_add_tail(struct list_head *new, struct list_head *head) >>>> +{ >>>> + list_add_tail(new, head); >>>> +} >>> >>> Just noticed, but of these helpers only `INIT_LIST_HEAD` and >>> `list_add_tail` seem to be used (in doccomments). >> >> Correct, but it makes sense to add the most obvious/common ones (also to make it clear that using these are supported). > > "It makes sense" is subjective, and in this case I am confident it is > not the right intuition to add dead code just for the sake of it. > > Each of these helpers adds a potential breakage point from the C API > should the latter change, so we should only add them if they are indeed > necessary. > > Actually, some of these helpers are not used when they could have been - > you have a `is_empty` method that rewrites the C function instead of > calling the helper. The only helpers that are unjustified as of now as > `list_add` and `list_del`, and these are easy to add when they become > necessary. > > But this raises an interesting dilemma: these helpers cannot be inlined > and add the overhead of a function call. On the other hand, the > definition of `list_head` is so excessively simple that manipulating it > directly is virtually as intuitive as invoking the helper - and doesn't > bear the overhead. So should we double-down on these helpers, or just > drop them completely and re-implement the list functionality we need for > increased performance?You do have a point that we have an abstraction leak anyway, so the question is do we need helpers at all. I am torn on this. On the one hand, if someone changes how list_head api works, then the rust side breaks (though that is easy to flag though via doc tests). On the other hand, we have the function call overhead you pointed and we are dealing with the pointers in rust directly anyway in some cases. Plus more often than not, new separate list variations (like hlist) are added for new use case than modifying existing ones, so it is just a small risk to directly reimplement list helpers in rust. I do like your idea though because it makes it a bit easier to add support for other list variants if needed (hlist, llist, RCU lists etc) since we don?t need wrappers per-se. Ok I guess I will drop all the helpers then unless there is a good reason to still keep them and see what it looks like. Some of them might have debugging code that?s on the side C that we may not want to skip. The RCU list api certainly does. Thanks.