bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Hangbin Liu" <liuhangbin@gmail.com>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"Magnus Karlsson" <magnus.karlsson@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	"Björn Töpel" <bjorn.topel@gmail.com>
Subject: Re: [PATCH RFC bpf-next 4/4] i40e: remove rcu_read_lock() around XDP program invocation
Date: Fri, 23 Apr 2021 22:33:29 +0200	[thread overview]
Message-ID: <87k0oseo6e.fsf@toke.dk> (raw)
In-Reply-To: <20210423135704.GC64904@ranger.igk.intel.com>

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

> On Fri, Apr 23, 2021 at 01:05:20PM +0200, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> The i40e driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP
>> program invocations. However, the actual lifetime of the objects referred
>> by the XDP program invocation is longer, all the way through to the call to
>> xdp_do_flush(), making the scope of the rcu_read_lock() too small. This
>> turns out to be harmless because it all happens in a single NAPI poll
>> cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock()
>> misleading.
> Okay, but what about the lifetime of the xdp_prog itself? Can xdp_prog
> change within a single NAPI poll? After reading previous discussions I
> would say it can't, right?

Well, bpf_prog objects are also RCU-protected so it's at least
guaranteed to stay alive until the end of the NAPI poll. But I don't
think there's anything preventing the program from being changed in the
middle of a NAPI poll.

> There are drivers that have a big RCU critical section in NAPI poll, but it
> seems that some read a xdp_prog a single time whereas others read it per
> processed frame.
> If we are sure that xdp_prog can't change on-the-fly then first low
> hanging fruit, at least for the Intel drivers, is to skip a test against
> NULL and read it only once at the beginning of NAPI poll. There might be
> also other micro-optimizations specific to each drivers that could be done
> based on that (that of course read the xdp_prog per each frame).

I think the main problem this could cause is that the dispatcher code
could have replaced the program in the dispatcher trampoline while the
driver was still using it, which would hurt performance. However,
ultimately this is under the control of the driver, since the program
install is a driver op. For instance, i40e_xdp_setup() does a
conditional synchronize_rcu() after removing a program; making this
unconditional (and maybe moving it after the writes to the rx_ring prog
pointers?) would ensure that the NAPI cycle had ended before the
bpf_op() call in dev_xdp_install(), which would delay the trampoline

I guess there could then be a window where the new program is being used
but has not been installed into the trampoline yet, then, so maybe
delaying that replace is not actually terribly important? Adding Björn,
maybe he has a better idea.

> Or am I nuts?

No I don't think so :)

I guess it remains to be seen whether there's a real performance
benefit, but at least I don't think there would be any safety or
correctness issues with attempting this.


      reply	other threads:[~2021-04-23 20:33 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-23 11:05 [PATCH RFC bpf-next 0/4] Clean up and document RCU-based object protection for XDP_REDIRECT Toke Høiland-Jørgensen
2021-04-23 11:05 ` [PATCH RFC bpf-next 1/4] rcu: Create an unrcu_pointer() to remove __rcu from a pointer Toke Høiland-Jørgensen
2021-04-23 11:05 ` [PATCH RFC bpf-next 2/4] dev: add rcu_read_lock_bh_held() as a valid check when getting a RCU dev ref Toke Høiland-Jørgensen
2021-04-23 11:05 ` [PATCH RFC bpf-next 3/4] xdp: add proper __rcu annotations to redirect map entries Toke Høiland-Jørgensen
2021-04-23 11:05 ` [PATCH RFC bpf-next 4/4] i40e: remove rcu_read_lock() around XDP program invocation Toke Høiland-Jørgensen
2021-04-23 13:57   ` Maciej Fijalkowski
2021-04-23 20:33     ` Toke Høiland-Jørgensen [this message]

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:

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

  git send-email \
    --in-reply-to=87k0oseo6e.fsf@toke.dk \
    --to=toke@redhat.com \
    --cc=bjorn.topel@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=hawk@kernel.org \
    --cc=kafai@fb.com \
    --cc=liuhangbin@gmail.com \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=paulmck@kernel.org \


* 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).