+ 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@xxxxxxxxxx>
...
> 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)) {
