Re: [PATCH net-next v1 1/2] net: introduce and use skb_frag_fill_page_desc()

[Simon Horman <simon.horman@corigine.com>]

On Wed, May 10, 2023 at 07:27:50AM -0700, Stanislav Fomichev wrote:
> On Wed, May 10, 2023 at 6:02 AM Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Wed, May 10, 2023 at 08:07:36PM +0800, Yunsheng Lin wrote:
> > > On 2023/5/10 16:40, Simon Horman wrote:
> > > > + XDP people and ML
> > > >
> > > > On Tue, May 09, 2023 at 07:41:45PM +0800, Yunsheng Lin wrote:
> > > >> Most users use __skb_frag_set_page()/skb_frag_off_set()/
> > > >> skb_frag_size_set() to fill the page desc for a skb frag.
> > > >>
> > > >> Introduce skb_frag_fill_page_desc() to do that.
> > > >>
> > > >> net/bpf/test_run.c does not call skb_frag_off_set() to
> > > >> set the offset, "copy_from_user(page_address(page), ...)"
> > > >> suggest that it is assuming offset to be initialized as
> > > >> zero, so call skb_frag_fill_page_desc() with offset being
> > > >> zero for this case.
> > > >
> > > > I think the question is, what is the value of bv_offset before this patch.
> > >
> > > sinfo seems to be part of the 'data' kzalloced in
> > > bpf_test_init(), so bv_offset should be zero too.
> >
> > Thanks, that sounds logical to me.
> 
> +1, doesn't look like we do anything special. We just allocate the
> page and assume zero offset.

Thanks. I'm happy with this patch now, FWIIW.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> > > > Lorenzo and Stanislav, do you have any insight here?
> > > >
> > > >>
> > > >> Also, skb_frag_set_page() is not used anymore, so remove
> > > >> it.
> > > >>
> > > >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> > > >
> > > > ...
> > > >
> > > >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > > >> index 738776ab8838..30be21c7d05f 100644
> > > >> --- a/include/linux/skbuff.h
> > > >> +++ b/include/linux/skbuff.h
> > > >> @@ -2411,6 +2411,15 @@ static inline unsigned int skb_pagelen(const struct sk_buff *skb)
> > > >>    return skb_headlen(skb) + __skb_pagelen(skb);
> > > >>  }
> > > >>
> > > >> +static inline void skb_frag_fill_page_desc(skb_frag_t *frag,
> > > >> +                                     struct page *page,
> > > >> +                                     int off, int size)
> > > >> +{
> > > >> +  frag->bv_page = page;
> > > >> +  frag->bv_offset = off;
> > > >
> > > > Maybe it is slightly nicer to use skb_frag_off_set() here.
> > >
> > > Yes, that is good idea.
> > > But we need to move the definition of skb_frag_off_set() before
> > > skb_frag_fill_page_desc in order to use it, I try to keep the
> > > patch simple for reviewing for now, so I perfer to not do it
> > > now if that is ok for you.
> >
> > Sure, that is fine by me.