From: Lai Jiangshan <hidden> Date: 2011-03-18 03:39:34
The rcu callback fc_rport_free_rcu() just calls a kfree(),
so we use kfree_rcu() instead of the call_rcu(fc_rport_free_rcu).
Signed-off-by: Lai Jiangshan <redacted>
---
drivers/scsi/libfc/fc_rport.c | 14 +-------------
1 files changed, 1 insertions(+), 13 deletions(-)
From: Robert Love <hidden> Date: 2011-03-22 17:28:36
On Thu, 2011-03-17 at 20:41 -0700, Lai Jiangshan wrote:
quoted hunk
The rcu callback fc_rport_free_rcu() just calls a kfree(),
so we use kfree_rcu() instead of the call_rcu(fc_rport_free_rcu).
Signed-off-by: Lai Jiangshan <redacted>
---
drivers/scsi/libfc/fc_rport.c | 14 +-------------
1 files changed, 1 insertions(+), 13 deletions(-)
From: Paul E. McKenney <hidden> Date: 2011-03-23 06:50:35
On Tue, Mar 22, 2011 at 10:28:33AM -0700, Robert Love wrote:
On Thu, 2011-03-17 at 20:41 -0700, Lai Jiangshan wrote:
quoted
The rcu callback fc_rport_free_rcu() just calls a kfree(),
so we use kfree_rcu() instead of the call_rcu(fc_rport_free_rcu).
Signed-off-by: Lai Jiangshan <redacted>
---
drivers/scsi/libfc/fc_rport.c | 14 +-------------
1 files changed, 1 insertions(+), 13 deletions(-)
I think this last line should be:
kfree_rcu(rdata, &rdata->rcu);
Hello, Robert,
I believe that it is correct as is. The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Then __kfree_rcu() encodes the offset into the rcu_head so that it can
be handled properly at the end of the grace period.
Thanx, Paul
I think this last line should be:
kfree_rcu(rdata, &rdata->rcu);
Hello, Robert,
I believe that it is correct as is. The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Then __kfree_rcu() encodes the offset into the rcu_head so that it can
be handled properly at the end of the grace period.
To be fair to Robert, I had the same thought, but decided to check the
definition of kfree_rcu before I made the same comment. Unfortunately,
the definition of kfree_rcu is still not in Linus' tree (and there's no
indication in the patch series about where to find the definition). It
would have been better if kfree_rcu were patch 1/n in the series, then
we'd've been able to review it more effectively.
Any idea when kfree_rcu will be submitted to Linus?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
I think this last line should be:
kfree_rcu(rdata, &rdata->rcu);
Hello, Robert,
I believe that it is correct as is. The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Then __kfree_rcu() encodes the offset into the rcu_head so that it can
be handled properly at the end of the grace period.
To be fair to Robert, I had the same thought, but decided to check the
definition of kfree_rcu before I made the same comment. Unfortunately,
the definition of kfree_rcu is still not in Linus' tree (and there's no
indication in the patch series about where to find the definition). It
would have been better if kfree_rcu were patch 1/n in the series, then
we'd've been able to review it more effectively.
Good point! That said, it is a bit ugly no matter how we handle it
because any of the patches that have not received acked-bys will need
to go up through their respective maintainer trees. Last time I had a
situation like this, it ended up stalling my commit stream for several
months, so was trying to separate them in order to avoid the stall.
But perhaps this is one of those situations where there is simply no
good way to handle the full series. :-(
Any idea when kfree_rcu will be submitted to Linus?
I have it queued, and it passes mild testing. I therefore expect to
push it for the next merge window.
So, do you guys want to carry the patches for your subsystems, or are
you willing to ack this one so that I can push it via -tip?
Thanx, Paul
From: James Bottomley <hidden> Date: 2011-03-23 14:06:07
On Tue, 2011-03-22 at 23:50 -0700, Paul E. McKenney wrote:
The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Isn't this one of those cases where the obvious use of the interface is
definitely wrong?
It's also another nasty pseudo C prototype. I know we do this sort of
thing for container_of et al, but I don't really think we want to extend
it.
Why not make the interface take a pointer to the embedding structure and
one to the rcu_head ... that way all pointer mathematics can be
contained inside the RCU routines.
James
From: Paul E. McKenney <hidden> Date: 2011-03-23 22:25:10
On Wed, Mar 23, 2011 at 09:05:51AM -0500, James Bottomley wrote:
On Tue, 2011-03-22 at 23:50 -0700, Paul E. McKenney wrote:
quoted
The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Isn't this one of those cases where the obvious use of the interface is
definitely wrong?
It's also another nasty pseudo C prototype. I know we do this sort of
thing for container_of et al, but I don't really think we want to extend
it.
Why not make the interface take a pointer to the embedding structure and
one to the rcu_head ... that way all pointer mathematics can be
contained inside the RCU routines.
Hello, James,
If you pass in a pair of pointers, then it is difficult for RCU to detect
bugs where the two pointers are unrelated. Yes, you can do some sanity
checks, but these get cumbersome and have corner cases where they can
be fooled. In contrast, Lai's interface allows the compiler to do the
needed type checking -- unless the second argument is a field of type
struct rcu_head in the structure pointed to by the first argument, the
compiler will complain.
Either way, the pointer mathematics are buried in the RCU API.
Or am I missing something here?
Thanx, Paul
From: James Bottomley <hidden> Date: 2011-03-23 22:45:47
On Wed, 2011-03-23 at 15:24 -0700, Paul E. McKenney wrote:
On Wed, Mar 23, 2011 at 09:05:51AM -0500, James Bottomley wrote:
quoted
On Tue, 2011-03-22 at 23:50 -0700, Paul E. McKenney wrote:
quoted
The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Isn't this one of those cases where the obvious use of the interface is
definitely wrong?
It's also another nasty pseudo C prototype. I know we do this sort of
thing for container_of et al, but I don't really think we want to extend
it.
Why not make the interface take a pointer to the embedding structure and
one to the rcu_head ... that way all pointer mathematics can be
contained inside the RCU routines.
Hello, James,
If you pass in a pair of pointers, then it is difficult for RCU to detect
bugs where the two pointers are unrelated. Yes, you can do some sanity
checks, but these get cumbersome and have corner cases where they can
be fooled. In contrast, Lai's interface allows the compiler to do the
needed type checking -- unless the second argument is a field of type
struct rcu_head in the structure pointed to by the first argument, the
compiler will complain.
Either way, the pointer mathematics are buried in the RCU API.
Or am I missing something here?
No ... I like the utility ... I just dislike the inelegance of having to
name a structure element in what looks like a C prototype.
I can see this proliferating everywhere since most of our reference
counting release callbacks basically free the enclosing object ...
James
Isn't this one of those cases where the obvious use of the interface is
definitely wrong?
But it's a compile time breakage if you use it wrong, not runtime.
It's also another nasty pseudo C prototype. I know we do this sort of
thing for container_of et al, but I don't really think we want to extend
it.
We do it for list_entry, list_for_each_entry, etc. And those are very
widespread within the kernel.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
From: Paul E. McKenney <hidden> Date: 2011-03-24 00:32:32
On Wed, Mar 23, 2011 at 05:45:32PM -0500, James Bottomley wrote:
On Wed, 2011-03-23 at 15:24 -0700, Paul E. McKenney wrote:
quoted
On Wed, Mar 23, 2011 at 09:05:51AM -0500, James Bottomley wrote:
quoted
On Tue, 2011-03-22 at 23:50 -0700, Paul E. McKenney wrote:
quoted
The kfree_rcu() definition is as
follows:
#define kfree_rcu(ptr, rcu_head) \
__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
Isn't this one of those cases where the obvious use of the interface is
definitely wrong?
It's also another nasty pseudo C prototype. I know we do this sort of
thing for container_of et al, but I don't really think we want to extend
it.
Why not make the interface take a pointer to the embedding structure and
one to the rcu_head ... that way all pointer mathematics can be
contained inside the RCU routines.
Hello, James,
If you pass in a pair of pointers, then it is difficult for RCU to detect
bugs where the two pointers are unrelated. Yes, you can do some sanity
checks, but these get cumbersome and have corner cases where they can
be fooled. In contrast, Lai's interface allows the compiler to do the
needed type checking -- unless the second argument is a field of type
struct rcu_head in the structure pointed to by the first argument, the
compiler will complain.
Either way, the pointer mathematics are buried in the RCU API.
Or am I missing something here?
No ... I like the utility ... I just dislike the inelegance of having to
name a structure element in what looks like a C prototype.
I can see this proliferating everywhere since most of our reference
counting release callbacks basically free the enclosing object ...
Indeed! Improvements are welcome -- it is just that I am not convinced
that the dual-pointer approach is really an improvement.
The C preprocessor... It is ugly, inelegant, painful, annoying, and
should have been strangled at birth -- but it is always there when you
need it!
Thanx, Paul