Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Ramamurthy, Harshitha" <harshitha.ramamurthy@intel.com>
To: David Ahern <dsahern@gmail.com>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>
Cc: "Duyck, Alexander H" <alexander.h.duyck@intel.com>,
	"Herbert, Tom" <tom.herbert@intel.com>
Subject: RE: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function
Date: Wed, 2 Sep 2020 00:52:09 +0000	[thread overview]
Message-ID: <MW3PR11MB45220F94F1E303CEC0F6C19C852F0@MW3PR11MB4522.namprd11.prod.outlook.com> (raw)
In-Reply-To: <04856bca-0952-4dd7-3313-a13be6b2e95a@gmail.com>

> From: bpf-owner@vger.kernel.org <bpf-owner@vger.kernel.org> On Behalf
> Of David Ahern
> Sent: Monday, August 31, 2020 12:54 PM
> To: Ramamurthy, Harshitha <harshitha.ramamurthy@intel.com>;
> bpf@vger.kernel.org; netdev@vger.kernel.org; ast@kernel.org;
> daniel@iogearbox.net; davem@davemloft.net; kuba@kernel.org
> Cc: Duyck, Alexander H <alexander.h.duyck@intel.com>; Herbert, Tom
> <tom.herbert@intel.com>
> Subject: Re: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function
> 
> On 8/31/20 1:25 PM, Harshitha Ramamurthy wrote:
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index
> > a613750d5515..bffe93b526e7 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3576,6 +3576,14 @@ 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.
> > + *
> > + * u32 bpf_get_xdp_hash(struct xdp_buff *xdp_md)
> 
> I thought there was a change recently making the uapi reference xdp_md;
> xdp_buff is not exported as part of the uapi.

Not sure what you mean - other xdp related helper functions still use xdp_buff as an argument. Could you point me to an example of what you are referring to?

> 
> 
> > + *	Description
> > + *		Return the hash for the xdp context passed. This function
> > + *		calls skb_flow_dissect in non-skb mode to calculate the
> > + *		hash for the packet.
> > + *	Return
> > + *		The 32-bit hash.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)		\
> >  	FN(unspec),			\
> > @@ -3727,6 +3735,7 @@ union bpf_attr {
> >  	FN(inode_storage_delete),	\
> >  	FN(d_path),			\
> >  	FN(copy_from_user),		\
> > +	FN(get_xdp_hash),		\
> >  	/* */
> >
> >  /* 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
> > 47eef9a0be6a..cfb5a6aea6c3 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3765,6 +3765,33 @@ static const struct bpf_func_proto
> bpf_xdp_redirect_map_proto = {
> >  	.arg3_type      = ARG_ANYTHING,
> >  };
> >
> > +BPF_CALL_1(bpf_get_xdp_hash, struct xdp_buff *, xdp) {
> > +	void *data_end = xdp->data_end;
> > +	struct ethhdr *eth = xdp->data;
> > +	void *data = xdp->data;
> > +	struct flow_keys keys;
> > +	u32 ret = 0;
> > +	int len;
> > +
> > +	len = data_end - data;
> > +	if (len <= 0)
> > +		return ret;
> 
> you should verify len covers the ethernet header. Looking at
> __skb_flow_dissect use of hlen presumes it exists.

Yes,  I will make sure to return if len < sizeof(struct ethhdr)

> 
> > +	memset(&keys, 0, sizeof(keys));
> > +	__skb_flow_dissect(dev_net(xdp->rxq->dev), NULL,
> &flow_keys_dissector,
> > +			   &keys, data, eth->h_proto, sizeof(*eth), len,
> > +			   FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
> 
> By STOP_AT_FLOW_LABEL I take it you want this to be an L3 hash. Why not
> add a flags argument to the helper and let the hash be L3 or L4?

I wrote this exactly how skb_get_hash calls skb_flow_dissect - with the same flag STOP_AT_FLOW_LABEL.  So it should already cover L3 and L4 hash, right? From what I understand STOP_AT_FLOW_LABEL flag is used to only stop parsing when a flow label is seen in ipv6 packets. 

> 
> 
> you should add test cases and have them cover the permutations - e.g., vlan,
> Q-in-Q, ipv4, ipv6, non-IP packet for L3 hash and then udp, tcp for L4 hash.

Sure, I will add test cases using this helper function.

Thanks for the feedback!
Harshitha


  reply	other threads:[~2020-09-02  0:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-31 19:25 Harshitha Ramamurthy
2020-08-31 19:54 ` David Ahern
2020-09-02  0:52   ` Ramamurthy, Harshitha [this message]
2020-09-02  2:09     ` David Ahern
2020-08-31 20:33 ` Daniel Borkmann
2020-09-02  0:54   ` Ramamurthy, Harshitha
2020-09-02 15:56   ` Tom Herbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MW3PR11MB45220F94F1E303CEC0F6C19C852F0@MW3PR11MB4522.namprd11.prod.outlook.com \
    --to=harshitha.ramamurthy@intel.com \
    --cc=alexander.h.duyck@intel.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tom.herbert@intel.com \
    --subject='RE: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).