From: Patrick McHardy <kaber@trash•net>
To: Herbert Xu <herbert@gondor•apana.org.au>
Cc: "Eric W. Biederman" <ebiederm@xmission•com>,
davem@davemloft•net, netdev@vger•kernel.org, oliver@neukum•name,
linux-usb-devel@lists•sourceforge.net
Subject: Re: [PATCH] rtnl: Simplify ASSERT_RTNL
Date: Mon, 08 Oct 2007 06:40:54 +0200 [thread overview]
Message-ID: <4709B4D6.3040805@trash.net> (raw)
In-Reply-To: <20071003060603.GA6457@gondor.apana.org.au>
Herbert Xu wrote:
> On Tue, Oct 02, 2007 at 05:29:11PM +0200, Patrick McHardy wrote:
>
>>I think this doesn't completely fix it, when dev_unicast_add is
>>interrupted by dev_mc_add before the unicast changes are performed,
>>they will get committed in the dev_mc_add context, so we might still
>>call change_flags with BH disabled. Taking the TX lock around the
>>dev->uc_count and dev->uc_promisc checks and changes in __dev_set_rx_mode
>>should fix this.
>
>
> Good catch. Digging back in history it seems that you added
> the change_rx_flags function so that the driver didn't have to
> do it under TX lock, right?
Yes, and to make sure the RTNL is held.
> The problem with this is that the stack can now call
> change_rx_flags and set_multicast_list simultaneously
> which presents a potential headache for the driver
> author (if they were to use change_rx_flags).
The change_rx_flags function was intended to be used by software
devices that want to synchronize flags to a different device,
in which case they shouldn't interfere since set_multicast_list
would only be used for the multicast list and not the flags.
> It seems to me what we could do is in fact separate out the
> part that adds the address and the part that syncs it with
> hardware.
That sounds like a really good idea to get rid of all the
confusion.
> That way we can call the hardware from a process context later
> and use the RTNL to guarantee that we only enter the driver
> once.
>
> So dev_mc_add would look like:
>
> 1) Hold some form of lock L.
> 2) Modify mc list A (a copy of the current mc list).
> 3) Drop lock.
> 4) Schedule an update to the hardware.
>
> The update to the hardware would look lie:
>
> 1) Hold RTNL.
> 2) Hold lock L.
> 3) Copy list A to list B (B would be our current list).
> 4) Drop lock L.
> 5) Call the hardware.
> 6) Drop RTNL.
>
> For compatibility, set_multicast_list would still be invoked
> under the TX lock while set_rx_mode would do exactly the same
> thing but would only hold the RTNL.
>
> What do you think about this approach?
Sounds great :)
next prev parent reply other threads:[~2007-10-08 4:43 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-29 0:59 [PATCH] rtnl: Simplify ASSERT_RTNL Eric W. Biederman
2007-09-29 4:31 ` Herbert Xu
2007-09-29 15:32 ` Patrick McHardy
2007-09-30 0:24 ` Herbert Xu
2007-09-30 15:47 ` Patrick McHardy
2007-10-02 9:28 ` Herbert Xu
2007-10-02 15:29 ` Patrick McHardy
2007-10-03 6:06 ` Herbert Xu
2007-10-08 4:40 ` Patrick McHardy [this message]
2007-09-29 17:18 ` Eric W. Biederman
2007-09-29 17:51 ` Patrick McHardy
2007-09-30 0:28 ` Herbert Xu
2007-10-11 4:16 ` David Miller
2007-10-11 6:57 ` Eric W. Biederman
2007-10-11 7:12 ` Herbert Xu
2007-10-11 8:23 ` Eric W. Biederman
2007-10-11 8:28 ` Herbert Xu
2007-10-11 16:33 ` Eric W. Biederman
2007-10-12 0:30 ` David Miller
2007-10-12 3:15 ` Eric W. Biederman
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=4709B4D6.3040805@trash.net \
--to=kaber@trash$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=ebiederm@xmission$(echo .)com \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=linux-usb-devel@lists$(echo .)sourceforge.net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oliver@neukum$(echo .)name \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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