Date: Mon, 19 Jun 2023 13:14:42 +0200
From: Greg KH <greg@...ah.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: alice@...l.io, andrew@...n.ch, kuba@...nel.org, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, aliceryhl@...gle.com,
	miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH 0/5] Rust abstractions for network device drivers

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@...ah.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@...l.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

Powered by blists - more mailing lists