[v3,net-next,06/12] bpf: helpers: add multibuffer support

Commit Message

Lorenzo Bianconi Sept. 30, 2020, 3:41 p.m. UTC

From: Sameeh Jubran <[e-mail]>

The implementation is based on this [0] draft by Jesper D. Brouer.

Provided two new helpers:

* bpf_xdp_get_frag_count()
* bpf_xdp_get_frags_total_size()

[0] xdp mb design - https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
Signed-off-by: Sameeh Jubran <[e-mail]>
Co-developed-by: Lorenzo Bianconi <[e-mail]>
Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
 include/uapi/linux/bpf.h       | 14 ++++++++++++
 net/core/filter.c              | 42 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++
 3 files changed, 70 insertions(+)

Comments

Alexei Starovoitov Sept. 30, 2020, 7:11 p.m. UTC | #1

On Wed, Sep 30, 2020 at 05:41:57PM +0200, Lorenzo Bianconi wrote:
> From: Sameeh Jubran <[e-mail]>
> 
> The implementation is based on this [0] draft by Jesper D. Brouer.
> 
> Provided two new helpers:
> 
> * bpf_xdp_get_frag_count()
> * bpf_xdp_get_frags_total_size()
> 
> [0] xdp mb design - https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
> Signed-off-by: Sameeh Jubran <[e-mail]>
> Co-developed-by: Lorenzo Bianconi <[e-mail]>
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
>  include/uapi/linux/bpf.h       | 14 ++++++++++++
>  net/core/filter.c              | 42 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 14 ++++++++++++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a22812561064..6f97dce8cccf 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3586,6 +3586,18 @@ union bpf_attr {
>   * 		the data in *dst*. This is a wrapper of **copy_from_user**\ ().
>   * 	Return
>   * 		0 on success, or a negative error in case of failure.
> + *
> + * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
> + *	Description
> + *		Get the number of fragments for a given xdp multi-buffer.
> + *	Return
> + *		The number of fragments
> + *
> + * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
> + *	Description
> + *		Get the total size of fragments for a given xdp multi-buffer.
> + *	Return
> + *		The total size of fragments for a given xdp multi-buffer.
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -3737,6 +3749,8 @@ union bpf_attr {
>  	FN(inode_storage_delete),	\
>  	FN(d_path),			\
>  	FN(copy_from_user),		\
> +	FN(xdp_get_frag_count),		\
> +	FN(xdp_get_frags_total_size),	\
>  	/* */

Please route the set via bpf-next otherwise merge conflicts will be severe.

Jesper Dangaard Brouer Oct. 1, 2020, 9:47 a.m. UTC | #2

On Wed, 30 Sep 2020 12:11:21 -0700
Alexei Starovoitov <[e-mail]> wrote:

> On Wed, Sep 30, 2020 at 05:41:57PM +0200, Lorenzo Bianconi wrote:
> > From: Sameeh Jubran <[e-mail]>
> > 
> > The implementation is based on this [0] draft by Jesper D. Brouer.

First of all I think you are giving me too much credit, and this is
both not really relevant and also not specific enough.  The link[0]
contains several proposals (actually from different people) and it is
not clear which of these proposal you reference.

I think this patch need to explain and argue why these BPF-helpers
makes sense... this will become BPF UAPI.

> > Provided two new helpers:
> > 
> > * bpf_xdp_get_frag_count()
> > * bpf_xdp_get_frags_total_size()

Why was the "frag" and "frags" name chosen?

 
> > [0] xdp mb design - https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
> > Signed-off-by: Sameeh Jubran <[e-mail]>
> > Co-developed-by: Lorenzo Bianconi <[e-mail]>
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> >  include/uapi/linux/bpf.h       | 14 ++++++++++++
> >  net/core/filter.c              | 42 ++++++++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h | 14 ++++++++++++
> >  3 files changed, 70 insertions(+)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index a22812561064..6f97dce8cccf 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3586,6 +3586,18 @@ union bpf_attr {
> >   * 		the data in *dst*. This is a wrapper of **copy_from_user**\ ().
> >   * 	Return
> >   * 		0 on success, or a negative error in case of failure.
> > + *
> > + * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
> > + *	Description
> > + *		Get the number of fragments for a given xdp multi-buffer.
> > + *	Return
> > + *		The number of fragments
> > + *
> > + * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
> > + *	Description
> > + *		Get the total size of fragments for a given xdp multi-buffer.
> > + *	Return
> > + *		The total size of fragments for a given xdp multi-buffer.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)		\
> >  	FN(unspec),			\
> > @@ -3737,6 +3749,8 @@ union bpf_attr {
> >  	FN(inode_storage_delete),	\
> >  	FN(d_path),			\
> >  	FN(copy_from_user),		\
> > +	FN(xdp_get_frag_count),		\
> > +	FN(xdp_get_frags_total_size),	\
> >  	/* */  
> 
> Please route the set via bpf-next otherwise merge conflicts will be severe.
>

Lorenzo Bianconi Oct. 1, 2020, 3:05 p.m. UTC | #3

> On Wed, Sep 30, 2020 at 05:41:57PM +0200, Lorenzo Bianconi wrote:

Hi Alexei,

> > From: Sameeh Jubran <[e-mail]>
> > 
> > The implementation is based on this [0] draft by Jesper D. Brouer.
> > 
> > Provided two new helpers:
> > 
> > * bpf_xdp_get_frag_count()
> > * bpf_xdp_get_frags_total_size()
> > 
> > + * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
> > + *	Description
> > + *		Get the number of fragments for a given xdp multi-buffer.
> > + *	Return
> > + *		The number of fragments
> > + *
> > + * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
> > + *	Description
> > + *		Get the total size of fragments for a given xdp multi-buffer.
> > + *	Return
> > + *		The total size of fragments for a given xdp multi-buffer.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)		\
> >  	FN(unspec),			\
> > @@ -3737,6 +3749,8 @@ union bpf_attr {
> >  	FN(inode_storage_delete),	\
> >  	FN(d_path),			\
> >  	FN(copy_from_user),		\
> > +	FN(xdp_get_frag_count),		\
> > +	FN(xdp_get_frags_total_size),	\
> >  	/* */
> 
> Please route the set via bpf-next otherwise merge conflicts will be severe.

ack, fine

in bpf-next the following two commits (available in net-next) are currently missing:
- 632bb64f126a: net: mvneta: try to use in-irq pp cache in mvneta_txq_bufs_free
- 879456bedbe5: net: mvneta: avoid possible cache misses in mvneta_rx_swbm

is it ok to rebase bpf-next ontop of net-next in order to post all the series
in bpf-next? Or do you prefer to post mvneta patches in net-next and bpf
related changes in bpf-next when it will rebased ontop of net-next?

Regards,
Lorenzo

Alexei Starovoitov Oct. 1, 2020, 3:40 p.m. UTC | #4

On Thu, Oct 1, 2020 at 8:05 AM Lorenzo Bianconi <[e-mail]> wrote:
>
> > On Wed, Sep 30, 2020 at 05:41:57PM +0200, Lorenzo Bianconi wrote:
>
> Hi Alexei,
>
> > > From: Sameeh Jubran <[e-mail]>
> > >
> > > The implementation is based on this [0] draft by Jesper D. Brouer.
> > >
> > > Provided two new helpers:
> > >
> > > * bpf_xdp_get_frag_count()
> > > * bpf_xdp_get_frags_total_size()
> > >
> > > + * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
> > > + * Description
> > > + *         Get the number of fragments for a given xdp multi-buffer.
> > > + * Return
> > > + *         The number of fragments
> > > + *
> > > + * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
> > > + * Description
> > > + *         Get the total size of fragments for a given xdp multi-buffer.
> > > + * Return
> > > + *         The total size of fragments for a given xdp multi-buffer.
> > >   */
> > >  #define __BPF_FUNC_MAPPER(FN)              \
> > >     FN(unspec),                     \
> > > @@ -3737,6 +3749,8 @@ union bpf_attr {
> > >     FN(inode_storage_delete),       \
> > >     FN(d_path),                     \
> > >     FN(copy_from_user),             \
> > > +   FN(xdp_get_frag_count),         \
> > > +   FN(xdp_get_frags_total_size),   \
> > >     /* */
> >
> > Please route the set via bpf-next otherwise merge conflicts will be severe.
>
> ack, fine
>
> in bpf-next the following two commits (available in net-next) are currently missing:
> - 632bb64f126a: net: mvneta: try to use in-irq pp cache in mvneta_txq_bufs_free
> - 879456bedbe5: net: mvneta: avoid possible cache misses in mvneta_rx_swbm
>
> is it ok to rebase bpf-next ontop of net-next in order to post all the series
> in bpf-next? Or do you prefer to post mvneta patches in net-next and bpf
> related changes in bpf-next when it will rebased ontop of net-next?

bpf-next will receive these patches later today,
so I prefer the whole thing on top of bpf-next at that time.

Lorenzo Bianconi Oct. 1, 2020, 3:44 p.m. UTC | #5

[...]
> > >
> > > Please route the set via bpf-next otherwise merge conflicts will be severe.
> >
> > ack, fine
> >
> > in bpf-next the following two commits (available in net-next) are currently missing:
> > - 632bb64f126a: net: mvneta: try to use in-irq pp cache in mvneta_txq_bufs_free
> > - 879456bedbe5: net: mvneta: avoid possible cache misses in mvneta_rx_swbm
> >
> > is it ok to rebase bpf-next ontop of net-next in order to post all the series
> > in bpf-next? Or do you prefer to post mvneta patches in net-next and bpf
> > related changes in bpf-next when it will rebased ontop of net-next?
> 
> bpf-next will receive these patches later today,
> so I prefer the whole thing on top of bpf-next at that time.

sounds good, thx.

Regards,
Lorenzo

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a22812561064..6f97dce8cccf 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3586,6 +3586,18 @@  union bpf_attr {
  * 		the data in *dst*. This is a wrapper of **copy_from_user**\ ().
  * 	Return
  * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the number of fragments for a given xdp multi-buffer.
+ *	Return
+ *		The number of fragments
+ *
+ * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of fragments for a given xdp multi-buffer.
+ *	Return
+ *		The total size of fragments for a given xdp multi-buffer.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3737,6 +3749,8 @@  union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_get_frag_count),		\
+	FN(xdp_get_frags_total_size),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/net/core/filter.c b/net/core/filter.c
index 706f8db0ccf8..7f33cfae219c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3475,6 +3475,44 @@  static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_xdp_get_frag_count, struct  xdp_buff*, xdp)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	return xdp->mb ? sinfo->nr_frags : 0;
+}
+
+const struct bpf_func_proto bpf_xdp_get_frag_count_proto = {
+	.func		= bpf_xdp_get_frag_count,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
+BPF_CALL_1(bpf_xdp_get_frags_total_size, struct  xdp_buff*, xdp)
+{
+	struct skb_shared_info *sinfo;
+	int nfrags, i, size = 0;
+
+	if (likely(!xdp->mb))
+		return 0;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	nfrags = min_t(u8, sinfo->nr_frags, MAX_SKB_FRAGS);
+
+	for (i = 0; i < nfrags; i++)
+		size += skb_frag_size(&sinfo->frags[i]);
+
+	return size;
+}
+
+const struct bpf_func_proto bpf_xdp_get_frags_total_size_proto = {
+	.func		= bpf_xdp_get_frags_total_size,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
 {
 	void *data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
@@ -6824,6 +6862,10 @@  xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_redirect_map_proto;
 	case BPF_FUNC_xdp_adjust_tail:
 		return &bpf_xdp_adjust_tail_proto;
+	case BPF_FUNC_xdp_get_frag_count:
+		return &bpf_xdp_get_frag_count_proto;
+	case BPF_FUNC_xdp_get_frags_total_size:
+		return &bpf_xdp_get_frags_total_size_proto;
 	case BPF_FUNC_fib_lookup:
 		return &bpf_xdp_fib_lookup_proto;
 #ifdef CONFIG_INET
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a22812561064..6f97dce8cccf 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3586,6 +3586,18 @@  union bpf_attr {
  * 		the data in *dst*. This is a wrapper of **copy_from_user**\ ().
  * 	Return
  * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the number of fragments for a given xdp multi-buffer.
+ *	Return
+ *		The number of fragments
+ *
+ * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of fragments for a given xdp multi-buffer.
+ *	Return
+ *		The total size of fragments for a given xdp multi-buffer.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3737,6 +3749,8 @@  union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_get_frag_count),		\
+	FN(xdp_get_frags_total_size),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper