From: Suzanne Wood <suzannew@cs.pdx.edu>
To: davem@davemloft.net
Cc: Robert.Olsson@data.slu.se, linux-kernel@vger.kernel.org,
paulmck@us.ibm.com, walpole@cs.pdx.edu
Subject: Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
Date: Tue, 27 Sep 2005 17:22:21 -0700 (PDT) [thread overview]
Message-ID: <200509280022.j8S0MLql000686@rastaban.cs.pdx.edu> (raw)
Many thanks for all you've provided here.
> From davem@davemloft.net Tue Sep 27 14:36:32 2005
> I agree with the changes to add rcu_dereference() use.
> Those were definitely lacking and needed.
> This following case is clever and correct, though. It is from
> the net/ipv4/devinet.c part of your patch:
> @@ -409,7 +412,8 @@ static int inet_rtm_deladdr(struct sk_bu
>
> if ((in_dev = inetdev_by_index(ifm->ifa_index)) == NULL)
> goto out;
> - __in_dev_put(in_dev);
> + in_dev_put(in_dev);
> + rcu_read_unlock();
>
> for (ifap = &in_dev->ifa_list; (ifa = *ifap) != NULL;
> ifap = &ifa->ifa_next) {
> Everyone gets fooled by a certain invariant in the Linux networking
> locking. If the RTNL semaphore is held, _all_ device and address
> configuration changes are blocked. IP addresses cannot be removed,
> devices cannot be brought up or down, routes cannot be added or
> deleted, etc. The RTNL semaphore serializes all of these operations.
> And it is held during inet_rtm_deladdr() here.
> So we _know_ that if inetdev_by_index() returns non-NULL someone
> else (the device itself) holds at least _one_ reference to that
> object which cannot go away, because all such actions would need
> to take the RTNL semaphore which we hold.
> So __in_dev_put() is safe here.
In this case, you want the refcnt decremented without the unnecessary
test that in_dev_put() would incur. I was concerned about the
pairings of __in_dev_get which doesn't increment refcnt with
__in_dev_put which decrements. Didn't mean to address that til
after some feedback, but thank you for clarifying my error here
since I can't trace any pairing with the use of __in_dev_put
in inet_rtm_deladdr.
> Arguably, it's being overly clever for questionable gain.
> It definitely deserves a comment, how about that? :-)
> Finally, about adding rcu_read_{lock,unlock}() around even
> in_dev_{get,put}(). I bet that really isn't needed but I cannot
> articulate why we can get away without it. For example, if we
> are given a pair executed in a function like:
> in_dev_get();
> ...
> in_dev_put();
> who cares if we preempt? The local function's execution holds the
> necessary reference, so the object's refcount cannot ever fall to
> zero.
> We can't get any RCU callbacks invoked, as a result, so we don't
> need the rcu_read_{lock,unlock}() calls here.
> The in_dev_put() uses atomic_dec_and_test(), which provides a memory
> barrier, so no out-of-order cpu memory references to the object
> can escape past the decrement to zero of the object reference count.
> In short, I think adding rcu_read_{lock,unlock}() is very heavy
> handed and unnecessary.
In Paul McKenney's reference at
www.rdrop.com/users/paulmck/RCU/whatisRCU.html
"Reference counts may be used in conjunction with RCU to maintain
longer-term references to data structures." So you're right. I
was basing those rcu_read_lock extents on the idea that the calling
function has the vision of the need for protection of an
rcu_dereference'd pointer. Paul has also provided further insight
into discriminating between read-side and update-side uses of
rcu_dereference which I need to incorporate.
Many thanks again and I'll try for a better submission.
next reply other threads:[~2005-09-28 0:22 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-09-28 0:22 Suzanne Wood [this message]
-- strict thread matches above, loose matches on Subject: below --
2005-10-01 18:37 [RFC][PATCH] identify in_dev_get rcu read-side critical sections Suzanne Wood
2005-10-01 19:29 ` Herbert Xu
2005-10-01 18:00 Suzanne Wood
2005-10-01 6:56 Suzanne Wood
2005-10-01 7:12 ` Herbert Xu
2005-10-01 18:04 ` Paul E. McKenney
2005-09-30 1:06 Suzanne Wood
2005-10-01 1:13 ` Herbert Xu
2005-09-29 23:59 Suzanne Wood
2005-09-30 0:23 ` Herbert Xu
2005-09-29 23:39 Suzanne Wood
2005-09-29 23:30 Suzanne Wood
2005-09-30 0:21 ` Herbert Xu
2005-09-30 0:23 ` Paul E. McKenney
2005-09-30 0:27 ` Herbert Xu
2005-09-30 0:36 ` Paul E. McKenney
2005-09-30 1:04 ` Herbert Xu
2005-09-30 1:16 ` Paul E. McKenney
2005-09-30 1:19 ` Herbert Xu
2005-09-29 16:02 Suzanne Wood
2005-09-29 21:28 ` Herbert Xu
2005-09-08 17:12 Suzanne Wood
2005-09-27 20:56 ` David S. Miller
2005-09-28 2:55 ` Herbert Xu
2005-09-28 14:51 ` Paul E. McKenney
2005-09-28 22:11 ` Herbert Xu
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=200509280022.j8S0MLql000686@rastaban.cs.pdx.edu \
--to=suzannew@cs.pdx.edu \
--cc=Robert.Olsson@data.slu.se \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@us.ibm.com \
--cc=walpole@cs.pdx.edu \
/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).