Re: [PATCH net-next v6 2/2] net: add netmem to skb_frag_t

[Mina Almasry <almasrymina@google.com>]

On Tue, Jan 30, 2024 at 1:34 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> I'm sorry for the late feedback.
>

Thanks for looking.

> On Tue, 2024-01-23 at 14:17 -0800, Mina Almasry wrote:
> > @@ -845,16 +863,24 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
> >  }
> >  EXPORT_SYMBOL(__napi_alloc_skb);
> >
> > -void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> > -                  int size, unsigned int truesize)
> > +void skb_add_rx_frag_netmem(struct sk_buff *skb, int i, netmem_ref netmem,
> > +                         int off, int size, unsigned int truesize)
> >  {
> >       DEBUG_NET_WARN_ON_ONCE(size > truesize);
> >
> > -     skb_fill_page_desc(skb, i, page, off, size);
> > +     skb_fill_netmem_desc(skb, i, netmem, off, size);
> >       skb->len += size;
> >       skb->data_len += size;
> >       skb->truesize += truesize;
> >  }
> > +EXPORT_SYMBOL(skb_add_rx_frag_netmem);
> > +
> > +void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> > +                  int size, unsigned int truesize)
> > +{
> > +     skb_add_rx_frag_netmem(skb, i, page_to_netmem(page), off, size,
> > +                            truesize);
> > +}
> >  EXPORT_SYMBOL(skb_add_rx_frag);
>
> Out of sheer ignorance, I'm unsure if the compiler will always inline
> the above skb_add_rx_frag_netmem() call. What about moving this helper
> to the header file?
>

Will do.

> > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> > index 1184d40167b8..145ef22b2b35 100644
> > --- a/net/kcm/kcmsock.c
> > +++ b/net/kcm/kcmsock.c
> > @@ -636,9 +636,14 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
> >               for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> >                       msize += skb_frag_size(&skb_shinfo(skb)->frags[i]);
> >
> > +             if (WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
> > +                     ret = -EINVAL;
> > +                     goto out;
> > +             }
>
> I feel like the following has been already discussed, but I could not
> find the relevant reference... Are all frags constrained to carry the
> same memref type? If not it would be better to move this check inside
> the previous loop, it's already traversing all the skb frags, it should
> not add measurable overhead.
>

Yes, this was discussed before. I believe the agreement is that, yes,
all the frags in a single skb will be constrained to a single type. It
was discussed on one of the many RFCs I believe.

Supporting skbs with mixed netmem types is certainly possible, but
requires per-frag checking and per-frag handling. Constraining all
skbs to the same netmem type just simplifies things greatly because
frag0 can be checked to determine the type of all the frags in the
skb, and all the frags in the skb can be processed the same as they're
the same type. There are no interesting use cases I can think of right
now that require mixed types, and the code can always be extended to
that if someone has a use case in the future.

I plan to add a WARN_ON_ONCE or DEBUG_NET_WARN_ON_ONCE in
skb_add_frag_rx_netmem that detects if the driver is trying to mix
types in the devmem series which adds non-page netmem.

If OK with you, I'll keep the check for only frag 0, but combine it
with the nr_frags check above like this:

diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 145ef22b2b35..73c200c5c8e4 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -627,7 +627,8 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
                        skb = txm->frag_skb;
                }

-               if (WARN_ON(!skb_shinfo(skb)->nr_frags)) {
+               if (WARN_ON(!skb_shinfo(skb)->nr_frags) ||
+                   WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
                        ret = -EINVAL;
                        goto out;
                }
@@ -636,11 +637,6 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
                for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
                        msize += skb_frag_size(&skb_shinfo(skb)->frags[i]);

-               if (WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
-                       ret = -EINVAL;
-                       goto out;
-               }
-

But I'm happy implementing the check exactly as you described if you
strongly prefer that instead, I don't think it's a big deal from my
end either way. Thanks!

-- 
Thanks,
Mina