All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eli Cohen <elic@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: <john.hurley@netronome.com>, <sriharsha.basavapatna@broadcom.com>,
	<ozsh@mellanox.com>, netdev <netdev@vger.kernel.org>
Subject: Re: Questioning requirement for ASSERT_RTNL in indirect code
Date: Tue, 14 Sep 2021 19:32:33 +0300	[thread overview]
Message-ID: <20210914163233.GA10664@mtl-vdi-166.wap.labs.mlnx> (raw)
In-Reply-To: <20210914080746.77ed3c7a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Tue, Sep 14, 2021 at 08:07:46AM -0700, Jakub Kicinski wrote:
> On Tue, 14 Sep 2021 17:54:39 +0300 Eli Cohen wrote:
> > On Tue, Sep 14, 2021 at 07:26:29AM -0700, Jakub Kicinski wrote:
> > > On Tue, 14 Sep 2021 09:05:00 +0300 Eli Cohen wrote:  
> > > > I see the same assert and the same comment, "All callback list access
> > > > should be protected by RTNL.", in the following locations
> > > > 
> > > > drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c:1873
> > > > drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c:303
> > > > drivers/net/ethernet/netronome/nfp/flower/offload.c:1770
> > > > 
> > > > I assume the source of this comment is the same. Can you guys explain
> > > > why is this necessary?  
> > > 
> > > Because most drivers (all but mlx5?) depend on rtnl_lock for
> > > serializing tc offload operations.
> > >   
> > 
> > But the assert I am referring to is called as part of setting up the
> > callback that will be used for offload operations, e.g. for adding a new
> > filter with tc. It's not the actual filter insetion code.
> > 
> > And as far as I can see this call sequence is already serialized by
> > flow_indr_block_lock.
> 
> Hm, indeed, should've looked at the code. There doesn't seem to be
> anything on the driver side this is protecting. The assert was added
> before the flow/nftables rewrite of the infra, perhaps that's the
> answer. IOW the lock did not exist back then.

ok, so if there are no objections by my next morning, I will post a
patch to remove these.

      reply	other threads:[~2021-09-14 16:32 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14  6:05 Questioning requirement for ASSERT_RTNL in indirect code Eli Cohen
2021-09-14 14:26 ` Jakub Kicinski
2021-09-14 14:54   ` Eli Cohen
2021-09-14 15:07     ` Jakub Kicinski
2021-09-14 16:32       ` Eli Cohen [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:
  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=20210914163233.GA10664@mtl-vdi-166.wap.labs.mlnx \
    --to=elic@nvidia.com \
    --cc=john.hurley@netronome.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ozsh@mellanox.com \
    --cc=sriharsha.basavapatna@broadcom.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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.