Subject: Re: [PATCHv14 bpf-next 3/6] xdp: add a new helper for dev map multicast support

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Mon, 18 Jan 2021 11:47:33 +0100

Hangbin Liu <[email-masked]> writes:

> 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/[email-masked]/

And then I corrected that after I figured out the real reason :)

https://lore.kernel.org/bpf/[email-masked]/ - Quote:

> The READ_ONCE() is not needed because the ex_map field is only ever read
> from or written to by the CPU owning the per-cpu pointer. Whereas the
> 'map' field is manipulated by remote CPUs in bpf_clear_redirect_map().
> So you need neither READ_ONCE() nor WRITE_ONCE() on ex_map, just like
> there are none on tgt_index and tgt_value.

-Toke