Re: [PATCH 0/5] Rust abstractions for network device drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

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,