From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Hangbin Liu <liuhangbin@gmail.com>, Martin KaFai Lau <kafai@fb.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>,
"David Ahern" <dsahern@gmail.com>,
"Andrii Nakryiko" <andrii.nakryiko@gmail.com>,
"Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
"John Fastabend" <john.fastabend@gmail.com>,
"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
"Björn Töpel" <bjorn.topel@gmail.com>
Subject: Re: [PATCHv7 bpf-next 2/4] xdp: extend xdp_redirect_map with broadcast support
Date: Thu, 15 Apr 2021 11:29:20 +0200 [thread overview]
Message-ID: <87lf9jkia7.fsf@toke.dk> (raw)
In-Reply-To: <20210415022127.GQ2900@Leo-laptop-t470s>
Hangbin Liu <liuhangbin@gmail.com> writes:
> On Wed, Apr 14, 2021 at 05:23:50PM -0700, Martin KaFai Lau wrote:
>> On Wed, Apr 14, 2021 at 08:26:08PM +0800, Hangbin Liu wrote:
>> [ ... ]
>>
>> > +static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifindex,
>> > + u64 flags, u64 flag_mask,
>> > void *lookup_elem(struct bpf_map *map, u32 key))
>> > {
>> > struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>> >
>> > /* Lower bits of the flags are used as return code on lookup failure */
>> > - if (unlikely(flags > XDP_TX))
>> > + if (unlikely(flags & ~(BPF_F_ACTION_MASK | flag_mask)))
>> > return XDP_ABORTED;
>> >
>> > ri->tgt_value = lookup_elem(map, ifindex);
>> > - if (unlikely(!ri->tgt_value)) {
>> > + if (unlikely(!ri->tgt_value) && !(flags & BPF_F_BROADCAST)) {
>> > /* If the lookup fails we want to clear out the state in the
>> > * redirect_info struct completely, so that if an eBPF program
>> > * performs multiple lookups, the last one always takes
>> > @@ -1482,13 +1484,21 @@ static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifind
>> > */
>> > ri->map_id = INT_MAX; /* Valid map id idr range: [1,INT_MAX[ */
>> > ri->map_type = BPF_MAP_TYPE_UNSPEC;
>> > - return flags;
>> > + return flags & BPF_F_ACTION_MASK;
>> > }
>> >
>> > ri->tgt_index = ifindex;
>> > ri->map_id = map->id;
>> > ri->map_type = map->map_type;
>> >
>> > + if (flags & BPF_F_BROADCAST) {
>> > + WRITE_ONCE(ri->map, map);
>> Why only WRITE_ONCE on ri->map? Is it needed?
>
> I think this is make sure the map pointer assigned to ri->map safely.
> which starts from commit f6069b9aa993 ("bpf: fix redirect to map under tail
> calls")
The reason WRITE_ONCE() is only on the map field is because that's the
one that could be changed by a remote CPU (in bpf_clear_redirect_map())
- everything else is only accessed on the local CPU.
As for whether it's strictly needed from a memory model PoV, I'm not
actually sure (and should we be using smp_{store_release,load_acquire}()
instead?); I view it mostly as an annotation to make it clear that the
map field is 'special' in this respect...
-Toke
next prev parent reply other threads:[~2021-04-15 9:29 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-14 12:26 [PATCHv7 bpf-next 0/4] xdp: extend xdp_redirect_map with broadcast support Hangbin Liu
2021-04-14 12:26 ` [PATCHv7 bpf-next 1/4] bpf: run devmap xdp_prog on flush instead of bulk enqueue Hangbin Liu
2021-04-15 0:17 ` Martin KaFai Lau
2021-04-15 2:37 ` Hangbin Liu
2021-04-15 9:22 ` Toke Høiland-Jørgensen
2021-04-15 17:35 ` Martin KaFai Lau
2021-04-15 18:21 ` Jesper Dangaard Brouer
2021-04-15 20:29 ` Toke Høiland-Jørgensen
2021-04-16 0:39 ` Martin KaFai Lau
2021-04-16 10:03 ` Toke Høiland-Jørgensen
2021-04-16 18:20 ` Martin KaFai Lau
2021-04-16 13:45 ` Jesper Dangaard Brouer
2021-04-16 14:35 ` Toke Høiland-Jørgensen
2021-04-16 18:22 ` Martin KaFai Lau
2021-04-17 0:23 ` Paul E. McKenney
2021-04-17 12:27 ` Toke Høiland-Jørgensen
2021-04-19 16:58 ` Paul E. McKenney
2021-04-19 18:12 ` Toke Høiland-Jørgensen
2021-04-19 18:32 ` Paul E. McKenney
2021-04-19 21:21 ` Toke Høiland-Jørgensen
2021-04-19 21:41 ` Paul E. McKenney
2021-04-19 22:16 ` Toke Høiland-Jørgensen
2021-04-19 22:31 ` Paul E. McKenney
2021-04-21 14:24 ` Toke Høiland-Jørgensen
2021-04-21 14:59 ` Paul E. McKenney
2021-04-21 19:59 ` Toke Høiland-Jørgensen
2021-04-21 20:51 ` Paul E. McKenney
2021-04-21 21:10 ` Toke Høiland-Jørgensen
2021-04-21 21:30 ` Paul E. McKenney
2021-04-21 22:00 ` Toke Høiland-Jørgensen
2021-04-21 22:31 ` Paul E. McKenney
2021-04-22 14:30 ` Toke Høiland-Jørgensen
2021-04-14 12:26 ` [PATCHv7 bpf-next 2/4] xdp: extend xdp_redirect_map with broadcast support Hangbin Liu
2021-04-15 0:23 ` Martin KaFai Lau
2021-04-15 2:21 ` Hangbin Liu
2021-04-15 9:29 ` Toke Høiland-Jørgensen [this message]
2021-04-14 12:26 ` [PATCHv7 bpf-next 3/4] sample/bpf: add xdp_redirect_map_multi for redirect_map broadcast test Hangbin Liu
2021-04-14 12:26 ` [PATCHv7 bpf-next 4/4] selftests/bpf: add xdp_redirect_multi test Hangbin Liu
2021-04-14 14:16 ` [PATCHv7 bpf-next 0/4] xdp: extend xdp_redirect_map with broadcast support Toke Høiland-Jørgensen
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=87lf9jkia7.fsf@toke.dk \
--to=toke@redhat.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bjorn.topel@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=daniel@iogearbox.net \
--cc=dsahern@gmail.com \
--cc=echaudro@redhat.com \
--cc=jbenc@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=liuhangbin@gmail.com \
--cc=lorenzo.bianconi@redhat.com \
--cc=maciej.fijalkowski@intel.com \
--cc=netdev@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).