Hi John,
Thanks for the reviewing.
On Sun, Jan 17, 2021 at 04:10:40PM -0800, John Fastabend wrote:
> > + * The forwarding *map* could be either BPF_MAP_TYPE_DEVMAP or
> > + * BPF_MAP_TYPE_DEVMAP_HASH. But the *ex_map* must be
> > + * BPF_MAP_TYPE_DEVMAP_HASH to get better performance.
>
> Would be good to add a note ex_map _must_ be keyed by ifindex for the
> helper to work. Its the obvious way to key a hashmap, but not required
> iirc.
OK, I will.
> > + if (!next_obj)
> > + last_one = true;
> > +
> > + if (last_one) {
> > + bq_enqueue(obj->dev, xdpf, dev_rx, obj->xdp_prog);
> > + return 0;
> > + }
>
> Just collapse above to
>
> if (!next_obj) {
> bq_enqueue()
> return
> }
>
> 'last_one' is a bit pointless here.
Yes, thanks.
> > @@ -3986,12 +3993,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> > {
> > struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> > struct bpf_map *map = READ_ONCE(ri->map);
> > + struct bpf_map *ex_map = ri->ex_map;
>
> READ_ONCE(ri->ex_map)?
>
> > u32 index = ri->tgt_index;
> > void *fwd = ri->tgt_value;
> > int err;
> >
> > ri->tgt_index = 0;
> > ri->tgt_value = NULL;
> > + ri->ex_map = NULL;
>
> WRITE_ONCE(ri->ex_map)?
>
> > WRITE_ONCE(ri->map, NULL);
>
> So we needed write_once, read_once pairs for ri->map do we also need them in
> the ex_map case?
Toke said this is no need for this read/write_once as there is already one.
https://lore.kernel.org/bpf/87r1wd2bqu.fsf@xxxxxxx/
Thanks
Hangbin
