BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Hangbin Liu <liuhangbin@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	Jiri Benc <jbenc@redhat.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	ast@kernel.org, Daniel Borkmann <daniel@iogearbox.net>,
	Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Subject: Re: [RFC PATCHv2 bpf-next 1/2] xdp: add a new helper for dev map multicast support
Date: Wed, 6 May 2020 17:14:42 +0800
Message-ID: <20200506091442.GA102436@dhcp-12-153.nay.redhat.com> (raw)
In-Reply-To: <87r1wd2bqu.fsf@toke.dk>

Hi Toke,

Thanks for your review, please see replies below.

On Fri, Apr 24, 2020 at 04:34:49PM +0200, Toke Høiland-Jørgensen wrote:
> >
> > The general data path is kept in net/core/filter.c. The native data
> > path is in kernel/bpf/devmap.c so we can use direct calls to
> > get better performace.
> 
> Got any performance numbers? :)

No, I haven't test the performance. Do you have any suggestions about how
to test it? I'd like to try forwarding pkts to 10+ ports. But I don't know
how to test the throughput. I don't think netperf or iperf supports this.
> 
> > + * int bpf_redirect_map_multi(struct bpf_map *map, struct bpf_map *ex_map, u64 flags)
> > + * 	Description
> > + * 		Redirect the packet to all the interfaces in *map*, and
> > + * 		exclude the interfaces that in *ex_map*. The *ex_map* could
> > + * 		be NULL.
> > + *
> > + * 		Currently the *flags* only supports *BPF_F_EXCLUDE_INGRESS*,
> > + * 		which could exlcude redirect to the ingress device.
> 
> I'd suggest rewording this to:
> 
> * 		Redirect the packet to ALL the interfaces in *map*, but
> * 		exclude the interfaces in *ex_map* (which may be NULL).
> *
> * 		Currently the *flags* only supports *BPF_F_EXCLUDE_INGRESS*,
> * 		which additionally excludes the current ingress device.

Thanks, I will update it
> > +
> > +bool dev_in_exclude_map(struct bpf_dtab_netdev *obj, struct bpf_map *map,
> > +			int exclude_ifindex)
> > +{
> > +	struct bpf_dtab_netdev *in_obj = NULL;
> > +	u32 key, next_key;
> > +	int err;
> > +
> > +	if (!map)
> > +		return false;
> > +
> > +	if (obj->dev->ifindex == exclude_ifindex)
> > +		return true;
> 
> We probably want the EXCLUDE_INGRESS flag to work even if ex_map is
> NULL, right? In that case you want to switch the order of the two checks
> above.

Yes, will fix it.

> 
> > +	devmap_get_next_key(map, NULL, &key);
> > +
> > +	for (;;) {
> 
> I wonder if we should require DEVMAP_HASH maps to be indexed by ifindex
> to avoid the loop?

I guess it's not easy to force user to index the map by ifindex.

> > +	xdpf = convert_to_xdp_frame(xdp);
> > +	if (unlikely(!xdpf))
> > +		return -EOVERFLOW;
> 
> You do a clone for each map entry below, so I think you end up leaking
> this initial xdpf? Also, you'll end up with one clone more than
> necessary - redirecting to two interfaces should only require 1 clone,
> you're doing 2.

We don't know which is the latest one. So we need to keep the initial
for clone. Is it enough to call xdp_release_frame() after the for loop?
> 
> > +	for (;;) {
> > +		switch (map->map_type) {
> > +		case BPF_MAP_TYPE_DEVMAP:
> > +			obj = __dev_map_lookup_elem(map, key);
> > +			break;
> > +		case BPF_MAP_TYPE_DEVMAP_HASH:
> > +			obj = __dev_map_hash_lookup_elem(map, key);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		if (!obj || dev_in_exclude_map(obj, ex_map,
> > +					       exclude_ingress ? dev_rx->ifindex : 0))
> > +			goto find_next;
> > +
> > +		dev = obj->dev;
> > +
> > +		if (!dev->netdev_ops->ndo_xdp_xmit)
> > +			return -EOPNOTSUPP;
> > +
> > +		err = xdp_ok_fwd_dev(dev, xdp->data_end - xdp->data);
> > +		if (unlikely(err))
> > +			return err;
> 
> These abort the whole operation midway through the loop if any error
> occurs. That is probably not what we want? I think the right thing to do
> is just continue the loop and only return an error if *all* of the
> forwarding attempts failed. Maybe we need a tracepoint to catch
> individual errors?

Makes sense. I will see if we can add a tracepoint here.
> >  
> > +static int dev_map_redirect_multi(struct net_device *dev, struct sk_buff *skb,
> > +				  struct bpf_prog *xdp_prog,
> > +				  struct bpf_map *map, struct bpf_map *ex_map,
> > +				  bool exclude_ingress)
> > +
> > +{
> > +	struct bpf_dtab_netdev *dst;
> > +	struct sk_buff *nskb;
> > +	u32 key, next_key;
> > +	int err;
> > +	void *fwd;
> > +
> > +	/* Get first key from forward map */
> > +	map->ops->map_get_next_key(map, NULL, &key);
> > +
> > +	for (;;) {
> > +		fwd = __xdp_map_lookup_elem(map, key);
> > +		if (fwd) {
> > +			dst = (struct bpf_dtab_netdev *)fwd;
> > +			if (dev_in_exclude_map(dst, ex_map,
> > +					       exclude_ingress ? dev->ifindex : 0))
> > +				goto find_next;
> > +
> > +			nskb = skb_clone(skb, GFP_ATOMIC);
> > +			if (!nskb)
> > +				return -EOVERFLOW;
> > +
> > +			err = dev_map_generic_redirect(dst, nskb, xdp_prog);
> > +			if (unlikely(err))
> > +				return err;
> > +		}
> > +
> > +find_next:
> > +		err = map->ops->map_get_next_key(map, &key, &next_key);
> > +		if (err)
> > +			break;
> > +
> > +		key = next_key;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> This duplication bugs me; maybe we should try to consolidate the generic
> and native XDP code paths?

Yes, I have tried to combine these two functions together. But one is generic
code path and another is XDP code patch. One use skb_clone and another
use xdpf_clone(). There are also some extra checks for XDP code. So maybe
we'd better just keep it as it is.

> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 2e29a671d67e..1dbe42290223 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> 
> Updates to tools/include should generally go into a separate patch.

Will fix it, thanks.

Best Regards
Hangbin

  reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-15  8:54 [RFC PATCH bpf-next 0/2] xdp: add " Hangbin Liu
2020-04-15  8:54 ` [RFC PATCH bpf-next 1/2] " Hangbin Liu
2020-04-20  9:52   ` Hangbin Liu
2020-04-15  8:54 ` [RFC PATCH bpf-next 2/2] sample/bpf: add xdp_redirect_map_multicast test Hangbin Liu
2020-04-24  8:56 ` [RFC PATCHv2 bpf-next 0/2] xdp: add dev map multicast support Hangbin Liu
2020-04-24  8:56   ` [RFC PATCHv2 bpf-next 1/2] xdp: add a new helper for " Hangbin Liu
2020-04-24 14:19     ` Lorenzo Bianconi
2020-04-28 11:09       ` Eelco Chaudron
2020-05-06  9:35       ` Hangbin Liu
2020-04-24 14:34     ` Toke Høiland-Jørgensen
2020-05-06  9:14       ` Hangbin Liu [this message]
2020-05-06 10:00         ` Toke Høiland-Jørgensen
2020-05-08  8:53           ` Hangbin Liu
2020-05-08 14:58             ` Toke Høiland-Jørgensen
2020-05-18  8:45       ` Hangbin Liu
2020-05-19 10:15         ` Jesper Dangaard Brouer
2020-05-20  1:24           ` Hangbin Liu
2020-04-24  8:56   ` [RFC PATCHv2 bpf-next 2/2] sample/bpf: add xdp_redirect_map_multicast test Hangbin Liu
2020-04-24 14:21     ` Lorenzo Bianconi
2020-05-23  6:05 ` [PATCHv3 bpf-next 0/2] xdp: add dev map multicast support Hangbin Liu
2020-05-23  6:05   ` [PATCHv3 bpf-next 1/2] xdp: add a new helper for " Hangbin Liu
2020-05-26  7:34     ` kbuild test robot
2020-05-23  6:05   ` [PATCHv3 bpf-next 2/2] sample/bpf: add xdp_redirect_map_multicast test Hangbin Liu
2020-05-26 14:05 ` [PATCHv4 bpf-next 0/2] xdp: add dev map multicast support Hangbin Liu
2020-05-26 14:05   ` [PATCHv4 bpf-next 1/2] xdp: add a new helper for " Hangbin Liu
2020-05-27 10:29     ` Toke Høiland-Jørgensen
2020-06-10 10:18     ` Jesper Dangaard Brouer
2020-06-12  8:54       ` Hangbin Liu
2020-06-16  8:55         ` Jesper Dangaard Brouer
2020-06-16 10:11           ` Hangbin Liu
2020-06-16 14:38             ` Jesper Dangaard Brouer
2020-06-10 10:21     ` Jesper Dangaard Brouer
2020-06-10 10:29       ` Toke Høiland-Jørgensen
2020-06-16  9:04         ` Jesper Dangaard Brouer
2020-05-26 14:05   ` [PATCHv4 bpf-next 2/2] sample/bpf: add xdp_redirect_map_multicast test Hangbin Liu
2020-05-27 10:21   ` [PATCHv4 bpf-next 0/2] xdp: add dev map multicast support Toke Høiland-Jørgensen
2020-05-27 10:32     ` Eelco Chaudron
2020-05-27 12:38     ` Hangbin Liu
2020-05-27 15:04       ` Toke Høiland-Jørgensen
2020-06-16  9:09         ` Jesper Dangaard Brouer
2020-06-16  9:47           ` Hangbin Liu
2020-06-03  2:40     ` Hangbin Liu
2020-06-03 11:05       ` Toke Høiland-Jørgensen
2020-06-04  4:09         ` Hangbin Liu
2020-06-04  9:44           ` Toke Høiland-Jørgensen
2020-06-04 12:12             ` Hangbin Liu
2020-06-04 12:37               ` Toke Høiland-Jørgensen
2020-06-04 14:41                 ` Hangbin Liu
2020-06-04 16:02                   ` Toke Høiland-Jørgensen
2020-06-05  6:26                     ` Hangbin Liu
2020-06-08 15:32                       ` Toke Høiland-Jørgensen
2020-06-09  3:03                         ` Hangbin Liu
2020-06-09 20:31                           ` Toke Høiland-Jørgensen
2020-06-10  2:35                             ` Hangbin Liu
2020-06-10 10:03                               ` Jesper Dangaard Brouer
2020-07-01  4:19   ` [PATCHv5 bpf-next 0/3] xdp: add a new helper for " Hangbin Liu
2020-07-01  4:19     ` [PATCHv5 bpf-next 1/3] " Hangbin Liu
2020-07-01  5:09       ` Andrii Nakryiko
2020-07-01  6:51         ` Hangbin Liu
2020-07-01 18:33       ` kernel test robot
2020-07-01  4:19     ` [PATCHv5 bpf-next 2/3] sample/bpf: add xdp_redirect_map_multicast test Hangbin Liu
2020-07-01  4:19     ` [PATCHv5 bpf-next 3/3] selftests/bpf: add xdp_redirect_multi test Hangbin Liu

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=20200506091442.GA102436@dhcp-12-153.nay.redhat.com \
    --to=liuhangbin@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=echaudro@redhat.com \
    --cc=jbenc@redhat.com \
    --cc=lorenzo.bianconi@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=toke@redhat.com \
    /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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git