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

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

[Simon Horman]

+ 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.

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.

> +	skb_frag_size_set(frag, size);
> +}
> +
>  static inline void __skb_fill_page_desc_noacc(struct skb_shared_info *shinfo,
>  					      int i, struct page *page,
>  					      int off, int size)
> @@ -2422,9 +2431,7 @@ static inline void __skb_fill_page_desc_noacc(struct skb_shared_info *shinfo,
>  	 * that not all callers have unique ownership of the page but rely
>  	 * on page_is_pfmemalloc doing the right thing(tm).
>  	 */
> -	frag->bv_page		  = page;
> -	frag->bv_offset		  = off;
> -	skb_frag_size_set(frag, size);
> +	skb_frag_fill_page_desc(frag, page, off, size);
>  }
>  
>  /**
> @@ -3496,20 +3503,6 @@ static inline void __skb_frag_set_page(skb_frag_t *frag, struct page *page)
>  	frag->bv_page = page;
>  }
>  
> -/**
> - * skb_frag_set_page - sets the page contained in a paged fragment of an skb
> - * @skb: the buffer
> - * @f: the fragment offset
> - * @page: the page to set
> - *
> - * Sets the @f'th fragment of @skb to contain @page.
> - */
> -static inline void skb_frag_set_page(struct sk_buff *skb, int f,
> -				     struct page *page)
> -{
> -	__skb_frag_set_page(&skb_shinfo(skb)->frags[f], page);
> -}
> -
>  bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t prio);
>  
>  /**
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index e79e3a415ca9..98143b86a9dd 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -1415,11 +1415,10 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
>  			}
>  
>  			frag = &sinfo->frags[sinfo->nr_frags++];
> -			__skb_frag_set_page(frag, page);
>  
>  			data_len = min_t(u32, kattr->test.data_size_in - size,
>  					 PAGE_SIZE);
> -			skb_frag_size_set(frag, data_len);
> +			skb_frag_fill_page_desc(frag, page, 0, data_len);
>  
>  			if (copy_from_user(page_address(page), data_in + size,
>  					   data_len)) {

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

[Yunsheng Lin]

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.

> 
> 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.

> 

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

[Simon Horman]

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.

> > 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.

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

[Stanislav Fomichev]

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.

> > > 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.

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

[Simon Horman]

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.