| Message ID | 20230613045326.3938283-1-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust abstractions for network device drivers | expand |
On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote: > This patchset adds minimum Rust abstractions for network device > drivers and an example of a Rust network device driver, a simpler > version of drivers/net/dummy.c. > > The dummy network device driver doesn't attach any bus such as PCI so > the dependency is minimum. Hopefully, it would make reviewing easier. > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml. > Hopefully, I've addressed all the issues. First things first, what are the current expectations for subsystems accepting rust code? I was hoping someone from the Rust side is going to review this. We try to review stuff within 48h at netdev, and there's no review :S My immediate instinct is that I'd rather not merge toy implementations unless someone within the netdev community can vouch for the code. > rust/bindings/bindings_helper.h | 3 + > rust/helpers.c | 23 ++ > rust/kernel/lib.rs | 3 + > rust/kernel/net.rs | 5 + > rust/kernel/net/dev.rs | 697 ++++++++++++++++++++++++++++++++ > samples/rust/Kconfig | 12 + > samples/rust/Makefile | 1 + > samples/rust/rust_net_dummy.rs | 81 ++++ > scripts/Makefile.build | 2 +- You seem to create a rust/net/ directory without adding anything to MAINTAINERS. Are we building a parallel directory structure? Are the maintainers also different?
On Thu, Jun 15, 2023 at 8:01 AM Jakub Kicinski <kuba@kernel.org> wrote: > > First things first, what are the current expectations for subsystems > accepting rust code? If you mean generally: it is up to each subsystem to decide whether to accept (or not) Rust code. Though we ask maintainers to make an effort to be flexible when things are so "core" that they could potentially block all other work, in order to conduct the Rust experiment and to try things out. On our side, we have a few guidelines for contributors [1]. [1] https://rust-for-linux.com/contributing#the-rust-subsystem > I was hoping someone from the Rust side is going to review this. > We try to review stuff within 48h at netdev, and there's no review :S I think the version number got reset, but Tomonori had a couple versions on the rust-for-linux@vger list [2][3]. Andrew Lunn was taking a look, and there were some other comments going on, too. The email threading is broken in [2][3], though, so it may be easiest to use a query like "f:lunn" [4] to find those. [2] https://lore.kernel.org/rust-for-linux/01010188843258ec-552cca54-4849-4424-b671-7a5bf9b8651a-000000@us-west-2.amazonses.com/ [3] https://lore.kernel.org/rust-for-linux/01010188a42d5244-fffbd047-446b-4cbf-8a62-9c036d177276-000000@us-west-2.amazonses.com/ [4] https://lore.kernel.org/rust-for-linux/?q=f%3Alunn > My immediate instinct is that I'd rather not merge toy implementations > unless someone within the netdev community can vouch for the code. Yes, in general, the goal is that maintainers actually understand what is getting merged, get involved, etc. So patch submitters of Rust code, at this time, should be expected/ready to explain Rust if needed. We can also help from the Rust subsystem side on that. But, yeah, knowledgeable people should review the code. > You seem to create a rust/net/ directory without adding anything > to MAINTAINERS. Are we building a parallel directory structure? > Are the maintainers also different? The plan is to split the `kernel` crate and move the files to their proper subsystems if the experiment goes well. But, indeed, it is best if a `F:` entry is added wherever you think it is best. Some subsystems may just add it to their entry (e.g. KUnit wants to do that). Others may decide to split the Rust part into another entry, so that maintainers may be a subset (or a different set -- sometimes this could be done, for instance, if a new maintainer shows up that wants to take care of the Rust abstractions). Cheers, Miguel
On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote: > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote: > > This patchset adds minimum Rust abstractions for network device > > drivers and an example of a Rust network device driver, a simpler > > version of drivers/net/dummy.c. > > > > The dummy network device driver doesn't attach any bus such as PCI so > > the dependency is minimum. Hopefully, it would make reviewing easier. > > > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml. > > Hopefully, I've addressed all the issues. > > First things first, what are the current expectations for subsystems > accepting rust code? > > I was hoping someone from the Rust side is going to review this. > We try to review stuff within 48h at netdev, and there's no review :S As pointed out elsewhere, i've looked the code over. I cannot say anything about the rust, but i don't see anything too obviously wrong with the way it use a few netdev API calls. > My immediate instinct is that I'd rather not merge toy implementations > unless someone within the netdev community can vouch for the code. It is definitely toy. But you have to start somewhere. What might be useful is an idea of the roadmap. How does this go from toy to something useful? I see two different threads of work. One is getting enough in place to find where rust is lacking. netdev uses a lot of inline C functions, which don't seem to map too well to Rust. And rust does not seem to have the concept of per CPU memory, needed for statistics. So it would be good to be able to have some code which can be profiled to see how bad this actually is, and then provide a test bed for finding solutions for these problems. The second is just wrapping more API calls to allow more capable drivers to be written. Implementing the loopback driver seems like the next obvious step. Then maybe a big jump to virtio_net? Like dummy and loopback, it is something which everybody can have, no physical hardware needed. It also has a lot of netdev features, NAPI, RSS, statistics, BPF & XDP. So it is a good playground, both for features and to profiling and performance optimisation. Andrew
On Thu, 15 Jun 2023 14:51:10 +0200 Andrew Lunn wrote: > On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote: > > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote: > > > This patchset adds minimum Rust abstractions for network device > > > drivers and an example of a Rust network device driver, a simpler > > > version of drivers/net/dummy.c. > > > > > > The dummy network device driver doesn't attach any bus such as PCI so > > > the dependency is minimum. Hopefully, it would make reviewing easier. > > > > > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml. > > > Hopefully, I've addressed all the issues. > > > > First things first, what are the current expectations for subsystems > > accepting rust code? > > > > I was hoping someone from the Rust side is going to review this. > > We try to review stuff within 48h at netdev, and there's no review :S > > As pointed out elsewhere, i've looked the code over. I cannot say > anything about the rust, but i don't see anything too obviously wrong > with the way it use a few netdev API calls. The skb freeing looks shady from functional perspective. I'm guessing some form of destructor frees the skb automatically in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason() is most certainly not safe to call on all xmit paths... > > My immediate instinct is that I'd rather not merge toy implementations > > unless someone within the netdev community can vouch for the code. > > It is definitely toy. But you have to start somewhere. > > What might be useful is an idea of the roadmap. How does this go from > toy to something useful? > > I see two different threads of work. > > One is getting enough in place to find where rust is lacking. netdev > uses a lot of inline C functions, which don't seem to map too well to > Rust. And rust does not seem to have the concept of per CPU memory, > needed for statistics. So it would be good to be able to have some > code which can be profiled to see how bad this actually is, and then > provide a test bed for finding solutions for these problems. > > The second is just wrapping more API calls to allow more capable > drivers to be written. Implementing the loopback driver seems like the > next obvious step. Then maybe a big jump to virtio_net? I'd have thought that a simple SPI driver or some such would be a better match. > Like dummy and loopback, it is something which everybody can have, no > physical hardware needed. It also has a lot of netdev features, NAPI, > RSS, statistics, BPF & XDP. So it is a good playground, both for > features and to profiling and performance optimisation. IMO we have too much useless playground stuff in C already, don't get me started. Let's get a real device prove that this thing can actually work :( We can waste years implementing imaginary device drivers that nobody ends up using.
On Thu, 15 Jun 2023 10:58:50 +0200 Miguel Ojeda wrote: > On Thu, Jun 15, 2023 at 8:01 AM Jakub Kicinski <kuba@kernel.org> wrote: > > I was hoping someone from the Rust side is going to review this. > > We try to review stuff within 48h at netdev, and there's no review :S > > I think the version number got reset, but Tomonori had a couple > versions on the rust-for-linux@vger list [2][3]. > > Andrew Lunn was taking a look, and there were some other comments going on, too. > > The email threading is broken in [2][3], though, so it may be easiest > to use a query like "f:lunn" [4] to find those. > > [2] https://lore.kernel.org/rust-for-linux/01010188843258ec-552cca54-4849-4424-b671-7a5bf9b8651a-000000@us-west-2.amazonses.com/ > [3] https://lore.kernel.org/rust-for-linux/01010188a42d5244-fffbd047-446b-4cbf-8a62-9c036d177276-000000@us-west-2.amazonses.com/ > [4] https://lore.kernel.org/rust-for-linux/?q=f%3Alunn > > > My immediate instinct is that I'd rather not merge toy implementations > > unless someone within the netdev community can vouch for the code. > > Yes, in general, the goal is that maintainers actually understand what > is getting merged, get involved, etc. So patch submitters of Rust > code, at this time, should be expected/ready to explain Rust if > needed. We can also help from the Rust subsystem side on that. > > But, yeah, knowledgeable people should review the code. All sounds pretty reasonable, thanks for the pointers. TBH I was hoping that the code will be more like reading "modern C++" for a C developer. I can't understand much of what's going on. Taking an example of what I have randomly on the screen as I'm writing this email: + /// Updates TX stats. + pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) { + // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay. + unsafe { + let inner = Opaque::get(&self.0); + (*inner).tx_packets = packets; + (*inner).tx_bytes = bytes; + (*inner).tx_errors = errors; + (*inner).tx_dropped = dropped; + } + } What is this supposed to be doing? Who needs to _set_ unrelated statistics from a function call? Yet no reviewer is complaining which either means I don't understand, or people aren't really paying attention :( > > You seem to create a rust/net/ directory without adding anything > > to MAINTAINERS. Are we building a parallel directory structure? > > Are the maintainers also different? > > The plan is to split the `kernel` crate and move the files to their > proper subsystems if the experiment goes well. I see. > But, indeed, it is best if a `F:` entry is added wherever you think it > is best. Some subsystems may just add it to their entry (e.g. KUnit > wants to do that). Others may decide to split the Rust part into > another entry, so that maintainers may be a subset (or a different set > -- sometimes this could be done, for instance, if a new maintainer > shows up that wants to take care of the Rust abstractions). I think F: would work for us. Are there success stories in any subsystem for getting a driver for real HW supported? I think the best way to focus the effort would be to set a target on a relatively simple device. Actually Andrew is interested, and PHY drivers seem relatively simple.. /me runs away
On Thu, Jun 15, 2023 at 07:02:52PM -0700, Jakub Kicinski wrote: > IMO we have too much useless playground stuff in C already, don't get me > started. Let's get a real device prove that this thing can actually work > :( We can waste years implementing imaginary device drivers that nobody > ends up using. Yes, the long standing policy seems to be not to merge "dummy" drivers. The original PTP patch set had one, but at the time the reviews said firmly "No" to that. Thanks, Richard
Hi, On Thu, 15 Jun 2023 19:19:31 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > TBH I was hoping that the code will be more like reading "modern C++" > for a C developer. I can't understand much of what's going on. > > Taking an example of what I have randomly on the screen as I'm writing > this email: > > + /// Updates TX stats. > + pub fn set_tx_stats(&mut self, packets: u64, bytes: u64, errors: u64, dropped: u64) { > + // SAFETY: We have exclusive access to the `rtnl_link_stats64`, so writing to it is okay. > + unsafe { > + let inner = Opaque::get(&self.0); > + (*inner).tx_packets = packets; > + (*inner).tx_bytes = bytes; > + (*inner).tx_errors = errors; > + (*inner).tx_dropped = dropped; > + } > + } > > What is this supposed to be doing? Who needs to _set_ unrelated > statistics from a function call? Yet no reviewer is complaining > which either means I don't understand, or people aren't really > paying attention :( Sorry, this function was used in the dummy driver to implement net_device_ops->ndo_get_stats64: https://lore.kernel.org/rust-for-linux/01010188a025632b-16a4fb69-5601-4f46-a170-52b5f2921ed2-000000@us-west-2.amazonses.com/T/#m518550baea9c76224524e44ab3ee5a0ecd01b1b9 The old version uses atomic types in xmit path to save tx stats for ndo_get_stats64. But Andrew said that using atomic types in xmit path isn't a good idea in even sample driver so I removed that code. >> But, indeed, it is best if a `F:` entry is added wherever you think it >> is best. Some subsystems may just add it to their entry (e.g. KUnit >> wants to do that). Others may decide to split the Rust part into >> another entry, so that maintainers may be a subset (or a different set >> -- sometimes this could be done, for instance, if a new maintainer >> shows up that wants to take care of the Rust abstractions). > > I think F: would work for us. > > Are there success stories in any subsystem for getting a driver for > real HW supported? I think the best way to focus the effort would be > to set a target on a relatively simple device. As far as I know, no subsystem has accepted Rust bindings yet. Replacing the existing C driver for real HW with Rust new one doesn't make sense, right? So a necessary condition of getting Rust bindings for a subsystem accepted is that a HW verndor implements both a driver and bindings for their new HW? thanks,
On 6/15/23 08:01, Jakub Kicinski wrote:> I was hoping someone from the Rust side is going to review this. > We try to review stuff within 48h at netdev, and there's no review :S I'm from the Rust side, and I intend to review this, but I've taken this week off, so it wont be until next week. > What is this supposed to be doing? Who needs to _set_ unrelated > statistics from a function call? Yet no reviewer is complaining > which either means I don't understand, or people aren't really > paying attention
Hi, On Thu, 15 Jun 2023 19:02:52 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 15 Jun 2023 14:51:10 +0200 Andrew Lunn wrote: >> On Wed, Jun 14, 2023 at 11:01:28PM -0700, Jakub Kicinski wrote: >> > On Tue, 13 Jun 2023 13:53:21 +0900 FUJITA Tomonori wrote: >> > > This patchset adds minimum Rust abstractions for network device >> > > drivers and an example of a Rust network device driver, a simpler >> > > version of drivers/net/dummy.c. >> > > >> > > The dummy network device driver doesn't attach any bus such as PCI so >> > > the dependency is minimum. Hopefully, it would make reviewing easier. >> > > >> > > Thanks a lot for reviewing on RFC patchset at rust-for-linux ml. >> > > Hopefully, I've addressed all the issues. >> > >> > First things first, what are the current expectations for subsystems >> > accepting rust code? >> > >> > I was hoping someone from the Rust side is going to review this. >> > We try to review stuff within 48h at netdev, and there's no review :S >> >> As pointed out elsewhere, i've looked the code over. I cannot say >> anything about the rust, but i don't see anything too obviously wrong >> with the way it use a few netdev API calls. > > The skb freeing looks shady from functional perspective. > I'm guessing some form of destructor frees the skb automatically > in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason() > is most certainly not safe to call on all xmit paths... Yeah, I assume that a driver keeps a skb in private data structure (such as tx ring) then removes the skb from it after the completion of tx; automatically the drop() method runs (where we need to free the skb). I thought that calling dev_kfree_skb() is fine but no? We also need something different for drivers that use other ways to free the skb though. I use kfree_skb_reason() because dev_kfree_skb() is a macro so it can't be called directly from Rust. But I should have used dev_kfree_skb() with a helper function. thanks,
> Actually Andrew is interested, and PHY drivers seem relatively simple.. > /me runs away :-) I think because they are so simple, there is not much to gain by implementing them in rust. Andrew
> I use kfree_skb_reason() because dev_kfree_skb() is a macro so it > can't be called directly from Rust. But I should have used > dev_kfree_skb() with a helper function. I think this is something you need to get addressed at a language level very soon. Lots of netdev API calls will be to macros. The API to manipulate skbs is pretty much always used on the hot path, so i expect that it will have a large number of macros. It is unclear to me how well it will scale if you need to warp them all? ~/linux/include/linux$ grep inline skbuff.h | wc 349 2487 23010 Do you really want to write 300+ wrappers? Andrew
On Fri, Jun 16, 2023 at 4:19 AM Jakub Kicinski <kuba@kernel.org> wrote: > > TBH I was hoping that the code will be more like reading "modern C++" > for a C developer. I can't understand much of what's going on. Yeah, there are some subtleties in the language, especially in `unsafe` land and dealing with FFI, but please feel free to ask. I think if one understands the complexities of modern C++, then high-level Rust can be mapped fairly well. > Are there success stories in any subsystem for getting a driver for > real HW supported? I think the best way to focus the effort would be > to set a target on a relatively simple device. DRM is looking into start merging things [1] soon, as far as I understand, and the GPU driver is being used in Asahi Linux. If we go a bit outside "real HW", but still "non-sample/toy modules", my understanding is that Binder is also looking into merging the Rust implementation in the next few months [2] (they are submitting the abstractions for their dependencies already). Then there are other efforts like the NVMe driver, which is progressing nicely and Andreas has shown nice performance results so far [3]. Though the upstreaming story for some of those efforts may be less clear (for different reasons). Of course, I don't speak for any of them -- I am just trying to give a summary of how things are going, and a positive outlook :) In the end, if a subsystem is willing to give things a try, we appreciate it, because it also makes things easier for others in the future, too. Perhaps you may want to treat it as an experiment (possibly marking things as experimental/staging/... explicitly if needed) that allows you to both learn Rust and whether it suits your subsystem, as well as a way to possibly get new people/future maintainers involved. [1] https://lore.kernel.org/rust-for-linux/20230307-rust-drm-v1-0-917ff5bc80a8@asahilina.net/ [2] https://rust-for-linux.com/android-binder-driver [3] https://rust-for-linux.com/nvme-driver Cheers, Miguel
> As for this being a single function rather than four functions, that's > definitely a debatable decision. You would only do that if it makes sense to > merge them together and if you would always assign all of them together. I > don't know enough about these fields to say whether it makes sense here. It can actually make sense to do them all together, because the source of these is likely to be a per CPU data structure protected by a per CPU sequence lock. You iterate over all CPUs, doing a transaction, taking the sequence lock, copy the values, and then releasing the lock. Taking and releases the lock per value is unnecessary expense. Andrew
On Fri, Jun 16, 2023 at 2:18 PM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > As far as I know, no subsystem has accepted Rust bindings yet. For abstractions in general (see my previous reply for "real HW" etc.), the KUnit subsystem [1] which is onboard and taking some patches through their tree / ownership of the code. [1] https://lore.kernel.org/rust-for-linux/CABVgOSnprvxzi-z42KFjOZsiRUv7u7E2poVGJNmTfS2OU4x4AA@mail.gmail.com/ > Replacing the existing C driver for real HW with Rust new one doesn't > make sense, right? So a necessary condition of getting Rust bindings > for a subsystem accepted is that a HW verndor implements both a driver > and bindings for their new HW? Not necessarily. It is true that, in general, the kernel does not want/accept duplicate implementations. However, this is a bit of a special situation, and there may be some reasons to allow for it in a given subsystem. For instance: - The need to experiment with Rust. - To have an actual in-tree user that allows to develop the abstractions for a subsystem, so that later they are ready to be used for future, actual new drivers. - Pending redesigns: sometimes subsystems may have a redesign/refactor/experiment that they have wanted to do for a while, so they may take the chance to also try to write it in Rust anyway. Of course, that could conflate two experiments, but... :) - Security: there may be some modules that have been problematic in the past (especially if due to memory safety issues / data races), and the subsystem may be willing to accept a parallel implementation to see if it would be an improvement thanks to Rust's properties. Cheers, Miguel
On 6/16/23 15:20, Andrew Lunn wrote: >> As for this being a single function rather than four functions, that's >> definitely a debatable decision. You would only do that if it makes sense to >> merge them together and if you would always assign all of them together. I >> don't know enough about these fields to say whether it makes sense here. > > It can actually make sense to do them all together, because the source > of these is likely to be a per CPU data structure protected by a per > CPU sequence lock. You iterate over all CPUs, doing a transaction, > taking the sequence lock, copy the values, and then releasing the > lock. Taking and releases the lock per value is unnecessary expense. It can probably be split into several methods without introducing a lock call for each one, if the API is designed right. Alice
Hi, On Fri, 16 Jun 2023 15:23:01 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Fri, Jun 16, 2023 at 2:18 PM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> As far as I know, no subsystem has accepted Rust bindings yet. > > For abstractions in general (see my previous reply for "real HW" > etc.), the KUnit subsystem [1] which is onboard and taking some > patches through their tree / ownership of the code. > > [1] https://lore.kernel.org/rust-for-linux/CABVgOSnprvxzi-z42KFjOZsiRUv7u7E2poVGJNmTfS2OU4x4AA@mail.gmail.com/ > >> Replacing the existing C driver for real HW with Rust new one doesn't >> make sense, right? So a necessary condition of getting Rust bindings >> for a subsystem accepted is that a HW verndor implements both a driver >> and bindings for their new HW? > > Not necessarily. It is true that, in general, the kernel does not > want/accept duplicate implementations. > > However, this is a bit of a special situation, and there may be some > reasons to allow for it in a given subsystem. For instance: > > - The need to experiment with Rust. > > - To have an actual in-tree user that allows to develop the > abstractions for a subsystem, so that later they are ready to be used > for future, actual new drivers. > > - Pending redesigns: sometimes subsystems may have a > redesign/refactor/experiment that they have wanted to do for a while, > so they may take the chance to also try to write it in Rust anyway. Of > course, that could conflate two experiments, but... :) > > - Security: there may be some modules that have been problematic in > the past (especially if due to memory safety issues / data races), and > the subsystem may be willing to accept a parallel implementation to > see if it would be an improvement thanks to Rust's properties. Yeah, I know :) But what I want to know is if there are such reasons for netdev subsystem, a necessary condition of getting Rust bindings accepted for the netdev maintainers. thanks,
On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote: > > I think this is something you need to get addressed at a language > level very soon. Lots of netdev API calls will be to macros. The API > to manipulate skbs is pretty much always used on the hot path, so i > expect that it will have a large number of macros. It is unclear to me > how well it will scale if you need to warp them all? > > ~/linux/include/linux$ grep inline skbuff.h | wc > 349 2487 23010 > > Do you really want to write 300+ wrappers? It would be very nice if at least `bindgen` (or even the Rust compiler... :) could cover many of these one-liners. We have discussed and asked for this in the past, and messages like this reinforce the need/request for this clearly, so thanks for this. Since `bindgen` 0.64.0 earlier this year [1] there is an experimental feature for this (`--wrap-static-fns`), so that is nice -- though we need to see how well it works. We are upgrading `bindgen` to the latest version after the merge window, so we can play with this soon. In particular, given: static inline int foo(int a, int b) { return a + b; } It generates a C file with e.g.: #include "a.h" // Static wrappers int foo__extern(int a, int b) { return foo(a, b); } And then in the usual Rust bindings: extern "C" { #[link_name = "foo__extern"] pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int) -> ::std::os::raw::c_int; } Cheers, Miguel
On Fri, Jun 16, 2023 at 03:48:31PM +0200, Miguel Ojeda wrote: > On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > I think this is something you need to get addressed at a language > > level very soon. Lots of netdev API calls will be to macros. The API > > to manipulate skbs is pretty much always used on the hot path, so i > > expect that it will have a large number of macros. It is unclear to me > > how well it will scale if you need to warp them all? > > > > ~/linux/include/linux$ grep inline skbuff.h | wc > > 349 2487 23010 > > > > Do you really want to write 300+ wrappers? > > It would be very nice if at least `bindgen` (or even the Rust > compiler... :) could cover many of these one-liners. We have discussed > and asked for this in the past, and messages like this reinforce the > need/request for this clearly, so thanks for this. > > Since `bindgen` 0.64.0 earlier this year [1] there is an experimental > feature for this (`--wrap-static-fns`), so that is nice -- though we > need to see how well it works. We are upgrading `bindgen` to the > latest version after the merge window, so we can play with this soon. > > In particular, given: > > static inline int foo(int a, int b) { > return a + b; > } > > It generates a C file with e.g.: > > #include "a.h" > > // Static wrappers > > int foo__extern(int a, int b) { return foo(a, b); } > > And then in the usual Rust bindings: > > extern "C" { > #[link_name = "foo__extern"] > pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int) > -> ::std::os::raw::c_int; > } I said in another email, i don't want to suggest premature optimisation, before profiling is done. But in C, these functions are inline for a reason. We don't want the cost of a subroutine call. We want the compiler to be able to inline the code, and the optimiser to be able to see it and generate the best code it can. Can the rust compile inline the binding including the FFI call? skb are used on the hot path. For a 10G Ethernet, it is dealing with nearly 1 million packets per second for big Ethernet frames, or worst case 14Mpps for small frames. Subroutine calls add up when you only have around 200 instructions to deal with a Ethernet frame. Andrew
On Fri, Jun 16, 2023 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote: > > I said in another email, i don't want to suggest premature > optimisation, before profiling is done. But in C, these functions are > inline for a reason. We don't want the cost of a subroutine call. We > want the compiler to be able to inline the code, and the optimiser to > be able to see it and generate the best code it can. See my reply in v2 to your message where I mentioned some of the options we are considering [1] -- not sure if you saw it: > Yeah, other use cases will also need that solved, e.g. Andreas for his > NVMe work. > > We discussed reimplementing performance-critical bits in Rust as you > suggest, as well as cross-language LTO. We also talked about possible > alternative approaches like "manual local LTO" for the helpers only > via feeding their LLVM IR to `rustc`, which may recover most of the > performance without having to go for full LTO and its associated > kernel link times. [1] https://lore.kernel.org/rust-for-linux/CANiq72kyUhvmG6KB32X1vuhNzOOJbs7R1JbK+vnPELX4tG73RA@mail.gmail.com/ Cheers, Miguel
On Thu, Jun 15, 2023 at 08:47:43PM -0700, Richard Cochran wrote: > On Thu, Jun 15, 2023 at 07:02:52PM -0700, Jakub Kicinski wrote: > > > IMO we have too much useless playground stuff in C already, don't get me > > started. Let's get a real device prove that this thing can actually work > > :( We can waste years implementing imaginary device drivers that nobody > > ends up using. > > Yes, the long standing policy seems to be not to merge "dummy" > drivers. The original PTP patch set had one, but at the time the > reviews said firmly "No" to that. Hi Richard I think in the modern day, you could probably add code to netdevsim to aid testing PTP. It should however by accompanied with selftests which actually make use of what you add. Andrew
On Fri, 16 Jun 2023 15:23:01 +0200 Miguel Ojeda wrote: > Not necessarily. It is true that, in general, the kernel does not > want/accept duplicate implementations. > > However, this is a bit of a special situation, and there may be some > reasons to allow for it in a given subsystem. For instance: > > - The need to experiment with Rust. Duplicated driver in a new language means nobody has a real incentive to use it in production. That really mutes the signal we get out of the experiment. At the same time IIUC building the Rust code is not trivial, so IDK if we're ready to force people to use it. Ugh. Do you have any idea how long it will take until one can dnf install $rust and have that be enough to be build a kernel (for the two major arches)?
On Fri, 16 Jun 2023 15:04:59 +0200 Andrew Lunn wrote: > > Actually Andrew is interested, and PHY drivers seem relatively simple.. > > /me runs away > > :-) > > I think because they are so simple, there is not much to gain by > implementing them in rust. I see many benefits :) Its a smallish and self-contained piece so it's achievable. Yet it interacts (to the extent the complexity of the PHY calls for it) with core netdev structures. Some PHYs even do timestamping which brings in interactions with skbs etc. Major benefit number 2 is that Rust is currently missing any real life bus bindings (AFAIU) and PHYs and the bus they live on are maintained by the same person
On Fri, 16 Jun 2023 22:02:20 +0900 (JST) FUJITA Tomonori wrote: > > The skb freeing looks shady from functional perspective. > > I'm guessing some form of destructor frees the skb automatically > > in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason() > > is most certainly not safe to call on all xmit paths... > > Yeah, I assume that a driver keeps a skb in private data structure > (such as tx ring) then removes the skb from it after the completion of > tx; automatically the drop() method runs (where we need to free the > skb). > > I thought that calling dev_kfree_skb() is fine but no? We also need > something different for drivers that use other ways to free the skb > though. > > I use kfree_skb_reason() because dev_kfree_skb() is a macro so it > can't be called directly from Rust. But I should have used > dev_kfree_skb() with a helper function. skbs (generally) can't be freed in an interrupt context. dev_kfree_skb_any_reason() is probably the most general implementation. But then we also have a set of functions used in known contexts for fast object recycling like napi_consume_skb(). How would complex object destruction rules fit in in the Rust world?
On 6/16/23 20:40, Jakub Kicinski wrote: > On Fri, 16 Jun 2023 22:02:20 +0900 (JST) FUJITA Tomonori wrote: >>> The skb freeing looks shady from functional perspective. >>> I'm guessing some form of destructor frees the skb automatically >>> in xmit handler(?), but (a) no reason support, (b) kfree_skb_reason() >>> is most certainly not safe to call on all xmit paths... >> >> Yeah, I assume that a driver keeps a skb in private data structure >> (such as tx ring) then removes the skb from it after the completion of >> tx; automatically the drop() method runs (where we need to free the >> skb). >> >> I thought that calling dev_kfree_skb() is fine but no? We also need >> something different for drivers that use other ways to free the skb >> though. >> >> I use kfree_skb_reason() because dev_kfree_skb() is a macro so it >> can't be called directly from Rust. But I should have used >> dev_kfree_skb() with a helper function. > > skbs (generally) can't be freed in an interrupt context. > dev_kfree_skb_any_reason() is probably the most general implementation. > But then we also have a set of functions used in known contexts for fast > object recycling like napi_consume_skb(). > How would complex object destruction rules fit in in the Rust world? A Rust method can be defined to take the struct "by value", which consumes the struct and prevents you from using it again. This can let you provide many different cleanup methods that each clean it up in different ways. However, you cannot force the user to use one of those methods. They always have the option of letting the value go out of scope, which calls the destructor. And they can do this at any time. That said, the destructor of the value does not necessarily *have* to translate to immediately freeing the value. If the value if refcounted, the destructor could just drop the refcount. It would also be possible for a destructor to schedule the cleanup operation to a workqueue. Or you could do something more clever. Alice
On Fri, 16 Jun 2023 21:00:36 +0200 Alice Ryhl wrote: > A Rust method can be defined to take the struct "by value", which > consumes the struct and prevents you from using it again. This can let > you provide many different cleanup methods that each clean it up in > different ways. > > However, you cannot force the user to use one of those methods. They > always have the option of letting the value go out of scope, which calls > the destructor. And they can do this at any time. > > That said, the destructor of the value does not necessarily *have* to > translate to immediately freeing the value. If the value if refcounted, > the destructor could just drop the refcount. It would also be possible > for a destructor to schedule the cleanup operation to a workqueue. Or > you could do something more clever. Can we put a WARN_ON() in the destructor and expect object to never be implicitly freed? skbs represent packets (most of the time) and for tracking which part of the stack is dropping packets we try to provide a drop reason along the freed skb. It'd be great if for Rust we could from the get-go direct everyone towards the APIs with an explicit reason code.
On 6/16/23 21:10, Jakub Kicinski wrote: > On Fri, 16 Jun 2023 21:00:36 +0200 Alice Ryhl wrote: >> A Rust method can be defined to take the struct "by value", which >> consumes the struct and prevents you from using it again. This can let >> you provide many different cleanup methods that each clean it up in >> different ways. >> >> However, you cannot force the user to use one of those methods. They >> always have the option of letting the value go out of scope, which calls >> the destructor. And they can do this at any time. >> >> That said, the destructor of the value does not necessarily *have* to >> translate to immediately freeing the value. If the value if refcounted, >> the destructor could just drop the refcount. It would also be possible >> for a destructor to schedule the cleanup operation to a workqueue. Or >> you could do something more clever. > > Can we put a WARN_ON() in the destructor and expect object to never be > implicitly freed? skbs represent packets (most of the time) and for > tracking which part of the stack is dropping packets we try to provide > a drop reason along the freed skb. It'd be great if for Rust we could > from the get-go direct everyone towards the APIs with an explicit reason > code. Yes, you can certainly put a WARN_ON in the destructor. Another possibility is to use a scope to clean up. I don't know anything about these skb objects are used, but you could have the user define a "process this socket" function that you pass a pointer to the skb, then make the return value be something that explains what should be done with the packet. Since you must return a value of the right type, this forces you to choose. Of course, this requires that the processing of packets can be expressed as a function call, where it only inspects the packet for the duration of that function call. (Lifetimes can ensure that the skb pointer does not escape the function.) Would something like that work? Alice
> Yes, you can certainly put a WARN_ON in the destructor. > > Another possibility is to use a scope to clean up. I don't know anything > about these skb objects are used, but you could have the user define a > "process this socket" function that you pass a pointer to the skb, then make > the return value be something that explains what should be done with the > packet. Since you must return a value of the right type, this forces you to > choose. > > Of course, this requires that the processing of packets can be expressed as > a function call, where it only inspects the packet for the duration of that > function call. (Lifetimes can ensure that the skb pointer does not escape > the function.) > > Would something like that work? I don't think so, at least not in the contest of an Rust Ethernet driver. There are two main flows. A packet is received. An skb is allocated and the received packet is placed into the skb. The Ethernet driver then hands the packet over to the network stack. The network stack is free to do whatever it wants with the packet. Things can go wrong within the driver, so at times it needs to free the skb rather than pass it to the network stack, which would be a drop. The second flow is that the network stack has a packet it wants sent out an Ethernet port, in the form of an skb. The skb gets passed to the Ethernet driver. The driver will do whatever it needs to do to pass the contents of the skb to the hardware. Once the hardware has it, the driver frees the skb. Again, things can go wrong and it needs to free the skb without sending it, which is a drop. So the lifetime is not a simple function call. The drop reason indicates why the packet was dropped. It should give some indication of what problem occurred which caused the drop. So ideally we don't want an anonymous drop. The C code does not enforce that, but it would be nice if the rust wrapper to dispose of an skb did enforce it. I would also say that this dummy driver and the C dummy driver is actually wrong in 'dropping' the frame. Its whole purpose in life is to be a black hole. It should only drop the packet if for some reason it cannot throw the packet into the black hole. Andrew
On Fri, Jun 16, 2023 at 8:26 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Duplicated driver in a new language means nobody has a real incentive > to use it in production. That really mutes the signal we get out of the > experiment. What I was trying to say is that there may be other incentives for using the new one, like the ones I gave in the list. Even if there is no incentive for using it right now, one may still want to build/design it in order to evaluate Rust and/or prepare abstractions for future drivers. This was the reason for the NVMe driver request we got back then, for instance. > At the same time IIUC building the Rust code is not trivial, > so IDK if we're ready to force people to use it. Ugh. I am not sure what you mean -- building works the same way as you have done now, i.e. your usual build commands with `make`. If you mean installing the toolchain, then we have a Quick Start guide [1], and a script that checks for the requirements. [1] https://docs.kernel.org/rust/quick-start.html > Do you have any idea how long it will take until one can > dnf install $rust > and have that be enough to be build a kernel (for the two major arches)? You may do it today if the version matches, which for Fedora it should e.g. next week or so when we merge the upgrade to 1.70, since Fedora has now 1.70 [2]. But since they may not always align, I would recommend using `rustup` (or the standalone installers) until we can establish a minimum version. Please see the Quick Start guide for details [1]. Other distributions may provide a custom package with the required dependencies for building Rust kernel code, like Ubuntu does. [2] https://packages.fedoraproject.org/pkgs/rust/rust/ Cheers, Miguel
On 6/16/23 22:04, Andrew Lunn wrote: >> Yes, you can certainly put a WARN_ON in the destructor. >> >> Another possibility is to use a scope to clean up. I don't know anything >> about these skb objects are used, but you could have the user define a >> "process this socket" function that you pass a pointer to the skb, then make >> the return value be something that explains what should be done with the >> packet. Since you must return a value of the right type, this forces you to >> choose. >> >> Of course, this requires that the processing of packets can be expressed as >> a function call, where it only inspects the packet for the duration of that >> function call. (Lifetimes can ensure that the skb pointer does not escape >> the function.) >> >> Would something like that work? > > I don't think so, at least not in the contest of an Rust Ethernet > driver. > > There are two main flows. > > A packet is received. An skb is allocated and the received packet is > placed into the skb. The Ethernet driver then hands the packet over to > the network stack. The network stack is free to do whatever it wants > with the packet. Things can go wrong within the driver, so at times it > needs to free the skb rather than pass it to the network stack, which > would be a drop. > > The second flow is that the network stack has a packet it wants sent > out an Ethernet port, in the form of an skb. The skb gets passed to > the Ethernet driver. The driver will do whatever it needs to do to > pass the contents of the skb to the hardware. Once the hardware has > it, the driver frees the skb. Again, things can go wrong and it needs > to free the skb without sending it, which is a drop. > > So the lifetime is not a simple function call. > > The drop reason indicates why the packet was dropped. It should give > some indication of what problem occurred which caused the drop. So > ideally we don't want an anonymous drop. The C code does not enforce > that, but it would be nice if the rust wrapper to dispose of an skb > did enforce it. It sounds like a destructor with WARN_ON is the best approach right now. Unfortunately, I don't think we can enforce that the destructor is not used today. That said, in the future it may be possible to implement a linter that detects it - I know that there have already been experiments with other custom lints for the kernel (e.g., enforcing that you don't sleep while holding a spinlock). > I would also say that this dummy driver and the C dummy driver is > actually wrong in 'dropping' the frame. Its whole purpose in life is to > be a black hole. It should only drop the packet if for some reason it > cannot throw the packet into the black hole. Ah, I suppose that we would also need a "by value" cleanup method for that case. Alice
On Sat, Jun 17, 2023 at 12:08:26PM +0200, Alice Ryhl wrote: > On 6/16/23 22:04, Andrew Lunn wrote: > > > Yes, you can certainly put a WARN_ON in the destructor. > > > > > > Another possibility is to use a scope to clean up. I don't know anything > > > about these skb objects are used, but you could have the user define a > > > "process this socket" function that you pass a pointer to the skb, then make > > > the return value be something that explains what should be done with the > > > packet. Since you must return a value of the right type, this forces you to > > > choose. > > > > > > Of course, this requires that the processing of packets can be expressed as > > > a function call, where it only inspects the packet for the duration of that > > > function call. (Lifetimes can ensure that the skb pointer does not escape > > > the function.) > > > > > > Would something like that work? > > > > I don't think so, at least not in the contest of an Rust Ethernet > > driver. > > > > There are two main flows. > > > > A packet is received. An skb is allocated and the received packet is > > placed into the skb. The Ethernet driver then hands the packet over to > > the network stack. The network stack is free to do whatever it wants > > with the packet. Things can go wrong within the driver, so at times it > > needs to free the skb rather than pass it to the network stack, which > > would be a drop. > > > > The second flow is that the network stack has a packet it wants sent > > out an Ethernet port, in the form of an skb. The skb gets passed to > > the Ethernet driver. The driver will do whatever it needs to do to > > pass the contents of the skb to the hardware. Once the hardware has > > it, the driver frees the skb. Again, things can go wrong and it needs > > to free the skb without sending it, which is a drop. > > > > So the lifetime is not a simple function call. > > > > The drop reason indicates why the packet was dropped. It should give > > some indication of what problem occurred which caused the drop. So > > ideally we don't want an anonymous drop. The C code does not enforce > > that, but it would be nice if the rust wrapper to dispose of an skb > > did enforce it. > > It sounds like a destructor with WARN_ON is the best approach right now. Note, if that WARN_ON fires, you just rebooted the machine as the huge majority of all Linux systems in the world run with "panic on warn" enabled :( So almost NEVER do a WARN* call if at all possible, unless there is no way back, and you can not recover from the mess, and crashing the system is your only recourse (instead of locking it up.) So please, don't do that, recover and report an error to the caller if possible and let them handle it properly. Yes, in a descructor that's almost impossible, but then perhaps if this is a real problem, something needs to be changed somewhere else in the callpath. thanks, greg k-h
Hi, On Sat, 17 Jun 2023 12:08:26 +0200 Alice Ryhl <alice@ryhl.io> wrote: > On 6/16/23 22:04, Andrew Lunn wrote: >>> Yes, you can certainly put a WARN_ON in the destructor. >>> >>> Another possibility is to use a scope to clean up. I don't know >>> anything >>> about these skb objects are used, but you could have the user define a >>> "process this socket" function that you pass a pointer to the skb, >>> then make >>> the return value be something that explains what should be done with >>> the >>> packet. Since you must return a value of the right type, this forces >>> you to >>> choose. >>> >>> Of course, this requires that the processing of packets can be >>> expressed as >>> a function call, where it only inspects the packet for the duration of >>> that >>> function call. (Lifetimes can ensure that the skb pointer does not >>> escape >>> the function.) >>> >>> Would something like that work? >> I don't think so, at least not in the contest of an Rust Ethernet >> driver. >> There are two main flows. >> A packet is received. An skb is allocated and the received packet is >> placed into the skb. The Ethernet driver then hands the packet over to >> the network stack. The network stack is free to do whatever it wants >> with the packet. Things can go wrong within the driver, so at times it >> needs to free the skb rather than pass it to the network stack, which >> would be a drop. >> The second flow is that the network stack has a packet it wants sent >> out an Ethernet port, in the form of an skb. The skb gets passed to >> the Ethernet driver. The driver will do whatever it needs to do to >> pass the contents of the skb to the hardware. Once the hardware has >> it, the driver frees the skb. Again, things can go wrong and it needs >> to free the skb without sending it, which is a drop. >> So the lifetime is not a simple function call. >> The drop reason indicates why the packet was dropped. It should give >> some indication of what problem occurred which caused the drop. So >> ideally we don't want an anonymous drop. The C code does not enforce >> that, but it would be nice if the rust wrapper to dispose of an skb >> did enforce it. > > It sounds like a destructor with WARN_ON is the best approach right > now. Better to simply BUG()? We want to make sure that a device driver explicity calls a function that consumes a skb object (on tx path, e.g., napi_consume_skb()). If a device driver doesn't call such, it's a bug that should be found easily and fixed during the development. It would be even better if the compiler could find such though. If Rust bindings for netdev could help device developpers in such way, it's worth an experiments? because looks like netdev subsystem accepts more drivers for new hardware than other subsystems. thanks,
On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote: > Hi, > > On Sat, 17 Jun 2023 12:08:26 +0200 > Alice Ryhl <alice@ryhl.io> wrote: > > > On 6/16/23 22:04, Andrew Lunn wrote: > >>> Yes, you can certainly put a WARN_ON in the destructor. > >>> > >>> Another possibility is to use a scope to clean up. I don't know > >>> anything > >>> about these skb objects are used, but you could have the user define a > >>> "process this socket" function that you pass a pointer to the skb, > >>> then make > >>> the return value be something that explains what should be done with > >>> the > >>> packet. Since you must return a value of the right type, this forces > >>> you to > >>> choose. > >>> > >>> Of course, this requires that the processing of packets can be > >>> expressed as > >>> a function call, where it only inspects the packet for the duration of > >>> that > >>> function call. (Lifetimes can ensure that the skb pointer does not > >>> escape > >>> the function.) > >>> > >>> Would something like that work? > >> I don't think so, at least not in the contest of an Rust Ethernet > >> driver. > >> There are two main flows. > >> A packet is received. An skb is allocated and the received packet is > >> placed into the skb. The Ethernet driver then hands the packet over to > >> the network stack. The network stack is free to do whatever it wants > >> with the packet. Things can go wrong within the driver, so at times it > >> needs to free the skb rather than pass it to the network stack, which > >> would be a drop. > >> The second flow is that the network stack has a packet it wants sent > >> out an Ethernet port, in the form of an skb. The skb gets passed to > >> the Ethernet driver. The driver will do whatever it needs to do to > >> pass the contents of the skb to the hardware. Once the hardware has > >> it, the driver frees the skb. Again, things can go wrong and it needs > >> to free the skb without sending it, which is a drop. > >> So the lifetime is not a simple function call. > >> The drop reason indicates why the packet was dropped. It should give > >> some indication of what problem occurred which caused the drop. So > >> ideally we don't want an anonymous drop. The C code does not enforce > >> that, but it would be nice if the rust wrapper to dispose of an skb > >> did enforce it. > > > > It sounds like a destructor with WARN_ON is the best approach right > > now. > > Better to simply BUG()? We want to make sure that a device driver > explicity calls a function that consumes a skb object (on tx path, > e.g., napi_consume_skb()). If a device driver doesn't call such, it's > a bug that should be found easily and fixed during the development. It > would be even better if the compiler could find such though. No, BUG() means "I have given up all hope here because the hardware is broken and beyond repair so the machine will now crash and take all of your data with it because I don't know how to properly recover". That should NEVER happen in a device driver, as that's very presumptious of it, and means the driver itself is broken. Report the error back up the chain and handle it properly, that's the correct thing to do. > If Rust bindings for netdev could help device developpers in such way, > it's worth an experiments? because looks like netdev subsystem accepts > more drivers for new hardware than other subsystems. Have you looked at the IIO subsystem? :) thanks, greg k-h
Hi, On Mon, 19 Jun 2023 11:46:38 +0200 Greg KH <greg@kroah.com> wrote: > On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote: >> Hi, >> >> On Sat, 17 Jun 2023 12:08:26 +0200 >> Alice Ryhl <alice@ryhl.io> wrote: >> >> > On 6/16/23 22:04, Andrew Lunn wrote: >> >>> Yes, you can certainly put a WARN_ON in the destructor. >> >>> >> >>> Another possibility is to use a scope to clean up. I don't know >> >>> anything >> >>> about these skb objects are used, but you could have the user define a >> >>> "process this socket" function that you pass a pointer to the skb, >> >>> then make >> >>> the return value be something that explains what should be done with >> >>> the >> >>> packet. Since you must return a value of the right type, this forces >> >>> you to >> >>> choose. >> >>> >> >>> Of course, this requires that the processing of packets can be >> >>> expressed as >> >>> a function call, where it only inspects the packet for the duration of >> >>> that >> >>> function call. (Lifetimes can ensure that the skb pointer does not >> >>> escape >> >>> the function.) >> >>> >> >>> Would something like that work? >> >> I don't think so, at least not in the contest of an Rust Ethernet >> >> driver. >> >> There are two main flows. >> >> A packet is received. An skb is allocated and the received packet is >> >> placed into the skb. The Ethernet driver then hands the packet over to >> >> the network stack. The network stack is free to do whatever it wants >> >> with the packet. Things can go wrong within the driver, so at times it >> >> needs to free the skb rather than pass it to the network stack, which >> >> would be a drop. >> >> The second flow is that the network stack has a packet it wants sent >> >> out an Ethernet port, in the form of an skb. The skb gets passed to >> >> the Ethernet driver. The driver will do whatever it needs to do to >> >> pass the contents of the skb to the hardware. Once the hardware has >> >> it, the driver frees the skb. Again, things can go wrong and it needs >> >> to free the skb without sending it, which is a drop. >> >> So the lifetime is not a simple function call. >> >> The drop reason indicates why the packet was dropped. It should give >> >> some indication of what problem occurred which caused the drop. So >> >> ideally we don't want an anonymous drop. The C code does not enforce >> >> that, but it would be nice if the rust wrapper to dispose of an skb >> >> did enforce it. >> > >> > It sounds like a destructor with WARN_ON is the best approach right >> > now. >> >> Better to simply BUG()? We want to make sure that a device driver >> explicity calls a function that consumes a skb object (on tx path, >> e.g., napi_consume_skb()). If a device driver doesn't call such, it's >> a bug that should be found easily and fixed during the development. It >> would be even better if the compiler could find such though. > > No, BUG() means "I have given up all hope here because the hardware is > broken and beyond repair so the machine will now crash and take all of > your data with it because I don't know how to properly recover". That > should NEVER happen in a device driver, as that's very presumptious of > it, and means the driver itself is broken. > > Report the error back up the chain and handle it properly, that's the > correct thing to do. I see. Then netdev_warn() should be used instead. Is it possible to handle the case where a device driver wrongly doesn't consume a skb object? >> If Rust bindings for netdev could help device developpers in such way, >> it's worth an experiments? because looks like netdev subsystem accepts >> more drivers for new hardware than other subsystems. > > Have you looked at the IIO subsystem? :) No, I've not. Are there possible drivers that Rust could be useful for? thanks,
On Mon, Jun 19, 2023 at 08:05:59PM +0900, FUJITA Tomonori wrote: > On Mon, 19 Jun 2023 11:46:38 +0200 > Greg KH <greg@kroah.com> wrote: > > > On Mon, Jun 19, 2023 at 05:50:03PM +0900, FUJITA Tomonori wrote: > >> Hi, > >> > >> On Sat, 17 Jun 2023 12:08:26 +0200 > >> Alice Ryhl <alice@ryhl.io> wrote: > >> > >> > On 6/16/23 22:04, Andrew Lunn wrote: > >> >>> Yes, you can certainly put a WARN_ON in the destructor. > >> >>> > >> >>> Another possibility is to use a scope to clean up. I don't know > >> >>> anything > >> >>> about these skb objects are used, but you could have the user define a > >> >>> "process this socket" function that you pass a pointer to the skb, > >> >>> then make > >> >>> the return value be something that explains what should be done with > >> >>> the > >> >>> packet. Since you must return a value of the right type, this forces > >> >>> you to > >> >>> choose. > >> >>> > >> >>> Of course, this requires that the processing of packets can be > >> >>> expressed as > >> >>> a function call, where it only inspects the packet for the duration of > >> >>> that > >> >>> function call. (Lifetimes can ensure that the skb pointer does not > >> >>> escape > >> >>> the function.) > >> >>> > >> >>> Would something like that work? > >> >> I don't think so, at least not in the contest of an Rust Ethernet > >> >> driver. > >> >> There are two main flows. > >> >> A packet is received. An skb is allocated and the received packet is > >> >> placed into the skb. The Ethernet driver then hands the packet over to > >> >> the network stack. The network stack is free to do whatever it wants > >> >> with the packet. Things can go wrong within the driver, so at times it > >> >> needs to free the skb rather than pass it to the network stack, which > >> >> would be a drop. > >> >> The second flow is that the network stack has a packet it wants sent > >> >> out an Ethernet port, in the form of an skb. The skb gets passed to > >> >> the Ethernet driver. The driver will do whatever it needs to do to > >> >> pass the contents of the skb to the hardware. Once the hardware has > >> >> it, the driver frees the skb. Again, things can go wrong and it needs > >> >> to free the skb without sending it, which is a drop. > >> >> So the lifetime is not a simple function call. > >> >> The drop reason indicates why the packet was dropped. It should give > >> >> some indication of what problem occurred which caused the drop. So > >> >> ideally we don't want an anonymous drop. The C code does not enforce > >> >> that, but it would be nice if the rust wrapper to dispose of an skb > >> >> did enforce it. > >> > > >> > It sounds like a destructor with WARN_ON is the best approach right > >> > now. > >> > >> Better to simply BUG()? We want to make sure that a device driver > >> explicity calls a function that consumes a skb object (on tx path, > >> e.g., napi_consume_skb()). If a device driver doesn't call such, it's > >> a bug that should be found easily and fixed during the development. It > >> would be even better if the compiler could find such though. > > > > No, BUG() means "I have given up all hope here because the hardware is > > broken and beyond repair so the machine will now crash and take all of > > your data with it because I don't know how to properly recover". That > > should NEVER happen in a device driver, as that's very presumptious of > > it, and means the driver itself is broken. > > > > Report the error back up the chain and handle it properly, that's the > > correct thing to do. > > I see. Then netdev_warn() should be used instead. Yes. > Is it possible to handle the case where a device driver wrongly > doesn't consume a skb object? I have no idea as I do not know the network driver stack, sorry. > >> If Rust bindings for netdev could help device developpers in such way, > >> it's worth an experiments? because looks like netdev subsystem accepts > >> more drivers for new hardware than other subsystems. > > > > Have you looked at the IIO subsystem? :) > > No, I've not. Are there possible drivers that Rust could be useful > for? Who knows, you said you were looking for subsystems with lots and lots of new drivers, and the IIO subsystem has over a thousand of them, all just added in the past few years. good luck! greg k-h
Hi Andrew, Miguel, On 6/16/23 16:43, Andrew Lunn wrote: > I said in another email, i don't want to suggest premature > optimisation, before profiling is done. But in C, these functions are > inline for a reason. We don't want the cost of a subroutine call. We > want the compiler to be able to inline the code, and the optimiser to > be able to see it and generate the best code it can. > > Can the rust compile inline the binding including the FFI call? This is possible, with cross-language LTO, see: https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html There are some requirements that need to happen for that to work (mainly, I believe, that the LLVM version used by rustc and clang agree). But in general it is possible. We use it extensively on Firefox. Of course the requirements of Firefox and the kernel might be different. I think we rely heavily on PGO instrumentation to make the linker inline ffi functions, but there might be other ways of forcing the linker to inline particular calls that bindgen could generate or what not. Cheers, -- Emilio
> > Report the error back up the chain and handle it properly, that's the > > correct thing to do. > > I see. Then netdev_warn() should be used instead. > > Is it possible to handle the case where a device driver wrongly > doesn't consume a skb object? The skb is then likely leaked. After a while, you will consume all the memory, the OOM handler will start killing processes, until eventually the machine dies. This is unlikely to happen on the main path, it would be a rather dumb developer who messed up something so obvious. Leaks are more likely to happen in the error paths, and that tends to happen less often, and so the machine is going to live for longer. But we are talking about different things here. Jakub wanted to ensure that when the driver does drop an skb, it includes a reason code indicating why it dropped it. If the driver tries to drop the frame without providing a reason, we want to know the code path, so it can be fixed. Even better if this can be done at compile time. Not releasing the frame at all is a different problem, and probably not easy to fix. There is some degree of handover of ownership of the skb. When asked to transmit it, the driver should eventually release the skb. However, that is often sometime in the future after the hardware has confirmed it has DMAed a copy of the frame into its own memory. On the receive side, in the normal path the driver could allocate an skb, setup the DMA to copy the frame into it, and then wait for an indication the DMA is complete. Then it passes it to the network stack, at which point the network stack becomes the owner. But there are no simple scope rules to detect an skb has been leaked. Andrew
.... > Not releasing the frame at all is a different problem, and probably > not easy to fix. There is some degree of handover of ownership of the > skb. When asked to transmit it, the driver should eventually release > the skb. However, that is often sometime in the future after the > hardware has confirmed it has DMAed a copy of the frame into its own > memory. On the receive side, in the normal path the driver could > allocate an skb, setup the DMA to copy the frame into it, and then > wait for an indication the DMA is complete. Then it passes it to the > network stack, at which point the network stack becomes the owner. > > But there are no simple scope rules to detect an skb has been leaked. You can require/enforce that the pointer the driver has is set to NULL when the skb is successfully passed on. But that tends to require passing the pointer by reference. Ok for long-lived items but a likely performance hit for skb. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, 17 Jun 2023 12:08:26 +0200 Alice Ryhl wrote: > > The drop reason indicates why the packet was dropped. It should give > > some indication of what problem occurred which caused the drop. So > > ideally we don't want an anonymous drop. The C code does not enforce > > that, but it would be nice if the rust wrapper to dispose of an skb > > did enforce it. > > It sounds like a destructor with WARN_ON is the best approach right now. > > Unfortunately, I don't think we can enforce that the destructor is not > used today. That said, in the future it may be possible to implement a > linter that detects it - I know that there have already been experiments > with other custom lints for the kernel (e.g., enforcing that you don't > sleep while holding a spinlock). Can we do something to hide the destructor from the linker?
On 6/20/23 17:47, Jakub Kicinski wrote: > On Sat, 17 Jun 2023 12:08:26 +0200 Alice Ryhl wrote: >>> The drop reason indicates why the packet was dropped. It should give >>> some indication of what problem occurred which caused the drop. So >>> ideally we don't want an anonymous drop. The C code does not enforce >>> that, but it would be nice if the rust wrapper to dispose of an skb >>> did enforce it. >> >> It sounds like a destructor with WARN_ON is the best approach right now. >> >> Unfortunately, I don't think we can enforce that the destructor is not >> used today. That said, in the future it may be possible to implement a >> linter that detects it - I know that there have already been experiments >> with other custom lints for the kernel (e.g., enforcing that you don't >> sleep while holding a spinlock). > > Can we do something to hide the destructor from the linker? We could probably have the destructor call some method that the linker wont be able to find, then mark the destructor with #[inline] so that dead-code elimination removes it if unused. (At least, in godbolt the inline marker was necessary for the destructor to get removed when not used.)
On Tue, Jun 20, 2023 at 6:55 PM Alice Ryhl <alice@ryhl.io> wrote: > > We could probably have the destructor call some method that the linker > wont be able to find, then mark the destructor with #[inline] so that > dead-code elimination removes it if unused. > > (At least, in godbolt the inline marker was necessary for the destructor > to get removed when not used.) Yeah, and we could use `build_assert!(false);` to ensure we don't ever call it (by users, or by the the custom destructor methods) -- it seems to work [1], but I am Cc'ing Gary in case he has some concerns (IIRC we discussed this in the past; I may be forgetting an issue with this). Cheers, Miguel [1] diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index c20b37e88ab2..7c313c75ff14 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -261,3 +261,28 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { // instead of `!`. See <https://github.com/rust-lang/rust-bindgen/issues/2094>. loop {} } + +/// Custom destructor example. +pub struct S; + +impl S { + /// Custom destructor 1. + pub fn close1(self) { + pr_info!("close1"); + core::mem::forget(self); // If commented out, it is a build error. + } + + /// Custom destructor 2. + pub fn close2(self) { + pr_info!("close2"); + core::mem::forget(self); // If commented out, it is a build error. + } +} + +impl Drop for S { + #[inline(always)] + fn drop(&mut self) { + // This should never be called. + build_assert!(false); + } +} diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs index 54de32b78cec..f07064ff6341 100644 --- a/samples/rust/rust_minimal.rs +++ b/samples/rust/rust_minimal.rs @@ -3,6 +3,7 @@ //! Rust minimal sample. use kernel::prelude::*; +use kernel::S; module! { type: RustMinimal, @@ -26,6 +27,12 @@ impl kernel::Module for RustMinimal { numbers.try_push(108)?; numbers.try_push(200)?; + let _s = S; + _s.close1(); // If commented out, it is a build error. + + let _s = S; + _s.close2(); // If commented out, it is a build error. + Ok(RustMinimal { numbers }) } }
On Tue, Jun 20, 2023 at 7:44 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Yeah, and we could use `build_assert!(false);` to ensure we don't ever > call it (by users, or by the the custom destructor methods) -- it i.e. `build_assert!` is a similar trick we have to produce a linker error like Alice suggested. It was "nicely packaged" by Gary a while ago :) Actually, it should be `build_error!("Normal destructor should never be called");` -- same thing (I just forgot we had that one). Cheers, Miguel
On Mon, Jun 19, 2023 at 1:27 PM Emilio Cobos Álvarez <emilio@crisal.io> wrote: > > Hi Andrew, Miguel, > > On 6/16/23 16:43, Andrew Lunn wrote: > > I said in another email, i don't want to suggest premature > > optimisation, before profiling is done. But in C, these functions are > > inline for a reason. We don't want the cost of a subroutine call. We > > want the compiler to be able to inline the code, and the optimiser to > > be able to see it and generate the best code it can. > > > > Can the rust compile inline the binding including the FFI call? > > This is possible, with cross-language LTO, see: > > https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html > > There are some requirements that need to happen for that to work > (mainly, I believe, that the LLVM version used by rustc and clang agree). > > But in general it is possible. We use it extensively on Firefox. Of > course the requirements of Firefox and the kernel might be different. > > I think we rely heavily on PGO instrumentation to make the linker inline > ffi functions, but there might be other ways of forcing the linker to > inline particular calls that bindgen could generate or what not. Thanks Emilio! It is nice to hear cross-language LTO is working well for Firefox. Andreas took a look at cross-language LTO some weeks ago, if I remember correctly (Cc'd). I am not sure about the latest status on kernel PGO, though. Cheers, Miguel
Hi All, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes: > On Mon, Jun 19, 2023 at 1:27 PM Emilio Cobos Álvarez <emilio@crisal.io> wrote: >> >> Hi Andrew, Miguel, >> >> On 6/16/23 16:43, Andrew Lunn wrote: >> > I said in another email, i don't want to suggest premature >> > optimisation, before profiling is done. But in C, these functions are >> > inline for a reason. We don't want the cost of a subroutine call. We >> > want the compiler to be able to inline the code, and the optimiser to >> > be able to see it and generate the best code it can. >> > >> > Can the rust compile inline the binding including the FFI call? >> >> This is possible, with cross-language LTO, see: >> >> https://blog.llvm.org/2019/09/closing-gap-cross-language-lto-between.html >> >> There are some requirements that need to happen for that to work >> (mainly, I believe, that the LLVM version used by rustc and clang agree). >> >> But in general it is possible. We use it extensively on Firefox. Of >> course the requirements of Firefox and the kernel might be different. >> >> I think we rely heavily on PGO instrumentation to make the linker inline >> ffi functions, but there might be other ways of forcing the linker to >> inline particular calls that bindgen could generate or what not. > > Thanks Emilio! It is nice to hear cross-language LTO is working well > for Firefox. > > Andreas took a look at cross-language LTO some weeks ago, if I > remember correctly (Cc'd). > > I am not sure about the latest status on kernel PGO, though. I hacked it to work a while back for the NVMe and null_blk drivers. You need to build the C and Rust parts of the kernel with compatible clang/rustc compilers (same major version I believe), and then pass the right compiler flags to get rustc and clang to emit llvm bitcode instead of ELF files with machine code. As far as I recall, some infrastructure is present in kbuild, but I had to add some bits to make it build. Also, I had some issues with LLVM not doing the inline properly and I had to use `llvm-link` on the bitcode of my module to link in the bitcode of the C code I wanted to inline. Without that step, inlining did not happen. I was able to build and execute in qemu like this, but I was not able to boot on bare metal. I think the LTO breaks something in C land. I did not investigate that further. Eventually I paused the work because I did not observe conclusive speedups in my benchmarks. I plan to resume the work at some point and do more rigorous benchmarking to see if I can observe a statistically significant speedup. Not sure when that will happen though. Best regards, Andreas
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes: > On Fri, Jun 16, 2023 at 3:14 PM Andrew Lunn <andrew@lunn.ch> wrote: >> >> I think this is something you need to get addressed at a language >> level very soon. Lots of netdev API calls will be to macros. The API >> to manipulate skbs is pretty much always used on the hot path, so i >> expect that it will have a large number of macros. It is unclear to me >> how well it will scale if you need to warp them all? >> >> ~/linux/include/linux$ grep inline skbuff.h | wc >> 349 2487 23010 >> >> Do you really want to write 300+ wrappers? > > It would be very nice if at least `bindgen` (or even the Rust > compiler... :) could cover many of these one-liners. We have discussed > and asked for this in the past, and messages like this reinforce the > need/request for this clearly, so thanks for this. > > Since `bindgen` 0.64.0 earlier this year [1] there is an experimental > feature for this (`--wrap-static-fns`), so that is nice -- though we > need to see how well it works. We are upgrading `bindgen` to the > latest version after the merge window, so we can play with this soon. > > In particular, given: > > static inline int foo(int a, int b) { > return a + b; > } > > It generates a C file with e.g.: > > #include "a.h" > > // Static wrappers > > int foo__extern(int a, int b) { return foo(a, b); } > > And then in the usual Rust bindings: > > extern "C" { > #[link_name = "foo__extern"] > pub fn foo(a: ::std::os::raw::c_int, b: ::std::os::raw::c_int) > -> ::std::os::raw::c_int; > } This is nice! It would be awesome if we could have something similar for macros. I am not sure if it is possible though. For the null_block demonstrator I had to move some C macros to Rust, for instance to implement iterators of bvec [1]. In the particular case of `bvec_iter_bvec()` it is not possible to wrap the macro in a function because the macro operates on a value, not a reference. We would have to pass an argument by value to the wrapping function in order to invoke the macro on this stack local variable, and then return the value again. Not really efficient. [1] https://lore.kernel.org/rust-for-linux/20230503090708.2524310-5-nmi@metaspace.dk/