Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, Mar 2, 2013 at 9:32 PM, Davidlohr Bueso wrote: > > After updating the callers, [msgctl, semctl, shmctl]_down, to acquire > the lock for IPC_RMID and IPC_SET commands, I'm no longer seeing these > issues - so far on my regular laptop and two big boxes running my Oracle > benchmarks for a few hours. Something like below (yes, I will address > the open coded spin_lock calls): Actually, please do me a favor, and do *not* change the semantics of these helper calls without changing the name of the helper. So I'd ask that instead of changing the callers, do this: - make the version that does *not* lock be called ipcctl_pre_down_nolock() - then, keep the old name, and just make it do the ipcctl_pre_down_nolock() followed by the spin_lock() (or rather, the non-open-coded one). Then, you can make each caller individually use the "nolock" version instead, and move the locking into the caller. But don't do the same mistake as the original patch, which was to change the locking semantics while keeping the same name of the function. In other words, it's much better to make these changes in small gradual pieces, and make each piece very obviously not change any semantics. So the first piece is the introduce the helper functions with new semantics (and new names), while keeping the old helpers with the old semantics and old names. Make it really clear that no semantics change. And the second piece is then to start using the split-up helpers that have different locking semantics and new names, and do it on a call-by-call basis. Ok? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, 2013-03-02 at 21:18 -0500, Rik van Riel wrote: > On 03/01/2013 11:32 PM, Michel Lespinasse wrote: > > > I think it may be nicer to take the rcu read lock at the call site > > rather than in ipc_obtain_object(), to make the rcu read lock/unlock > > sites pair up more nicely. Either that or make an inline > > ipc_release_object function that pairs up with ipc_obtain_object() and > > just does an rcu_read_unlock(). > > I started on a patch series to untangle the IPC locking, so > it will be a little more readable, and easier to maintain. > > It is a slower approach than Davidlohr's, as in, it will take > a little longer to put a patch series together, but I hope it > will be easier to debug... > > I hope to post a first iteration of the series by the middle > of next week. Goody, I'll be watching out for it. I have a big box rt user who uses semaphores in their very tight constraint application. While they're using them carefully, I saw a trace where contention cost vs jitter budget was a bit too high for comfort, and semctl/semop collision. -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, 2013-03-02 at 13:24 -0800, Linus Torvalds wrote:
> On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso
> wrote:
> > @@ -784,7 +806,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
> > ipc_namespace *ns,
> > int err;
> >
> > down_write(>rw_mutex);
> > - ipcp = ipc_lock_check(ids, id);
> > + ipcp = ipc_obtain_object_check(ids, id);
> > if (IS_ERR(ipcp)) {
> > err = PTR_ERR(ipcp);
> > goto out_up;
> > @@ -801,7 +823,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
> > ipc_namespace *ns,
> > return ipcp;
> >
> > err = -EPERM;
> > - ipc_unlock(ipcp);
> > + rcu_read_unlock();
> > out_up:
> > up_write(>rw_mutex);
> > return ERR_PTR(err);
>
> Uhhuh. This is very buggy, and I think it's the reason for the later
> bugs that Emmanuel reported.
Yes, quite buggy. I was able to mess up three different machines with
this, and since semaphores aren't the only users of ipcctl_pre_down(),
it could explain the sys_shmctl() call in the trace Emmanuel reported.
>
> In particular, the *non-error* case is buggy, where it in the middle
> of the function does
>
> return ipcp;
>
> for a successful lookup.
>
> It used to return a locked ipcp, now it no longer does. And you didn't
> change any of the callers, which still do the "ipc_unlock()" at the
> end. So all the locking gets completely confused.
>
After updating the callers, [msgctl, semctl, shmctl]_down, to acquire
the lock for IPC_RMID and IPC_SET commands, I'm no longer seeing these
issues - so far on my regular laptop and two big boxes running my Oracle
benchmarks for a few hours. Something like below (yes, I will address
the open coded spin_lock calls):
@@ -1101,16 +1138,20 @@ static int semctl_down(struct ipc_namespace *ns, int
semid,
switch(cmd){
case IPC_RMID:
+ spin_lock(>sem_perm.lock);
freeary(ns, ipcp);
goto out_up;
case IPC_SET:
+ spin_lock(>sem_perm.lock);
err = ipc_update_perm(_perm, ipcp);
if (err)
goto out_unlock;
sma->sem_ctime = get_seconds();
break;
default:
+ rcu_read_unlock();
err = -EINVAL;
+ goto out_up;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On 03/01/2013 11:32 PM, Michel Lespinasse wrote: I think it may be nicer to take the rcu read lock at the call site rather than in ipc_obtain_object(), to make the rcu read lock/unlock sites pair up more nicely. Either that or make an inline ipc_release_object function that pairs up with ipc_obtain_object() and just does an rcu_read_unlock(). I started on a patch series to untangle the IPC locking, so it will be a little more readable, and easier to maintain. It is a slower approach than Davidlohr's, as in, it will take a little longer to put a patch series together, but I hope it will be easier to debug... I hope to post a first iteration of the series by the middle of next week. -- All rights reversed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso wrote:
> @@ -784,7 +806,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
> ipc_namespace *ns,
> int err;
>
> down_write(>rw_mutex);
> - ipcp = ipc_lock_check(ids, id);
> + ipcp = ipc_obtain_object_check(ids, id);
> if (IS_ERR(ipcp)) {
> err = PTR_ERR(ipcp);
> goto out_up;
> @@ -801,7 +823,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
> ipc_namespace *ns,
> return ipcp;
>
> err = -EPERM;
> - ipc_unlock(ipcp);
> + rcu_read_unlock();
> out_up:
> up_write(>rw_mutex);
> return ERR_PTR(err);
Uhhuh. This is very buggy, and I think it's the reason for the later
bugs that Emmanuel reported.
In particular, the *non-error* case is buggy, where it in the middle
of the function does
return ipcp;
for a successful lookup.
It used to return a locked ipcp, now it no longer does. And you didn't
change any of the callers, which still do the "ipc_unlock()" at the
end. So all the locking gets completely confused.
Oops.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso davidlohr.bu...@hp.com wrote:
@@ -784,7 +806,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
ipc_namespace *ns,
int err;
down_write(ids-rw_mutex);
- ipcp = ipc_lock_check(ids, id);
+ ipcp = ipc_obtain_object_check(ids, id);
if (IS_ERR(ipcp)) {
err = PTR_ERR(ipcp);
goto out_up;
@@ -801,7 +823,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
ipc_namespace *ns,
return ipcp;
err = -EPERM;
- ipc_unlock(ipcp);
+ rcu_read_unlock();
out_up:
up_write(ids-rw_mutex);
return ERR_PTR(err);
Uhhuh. This is very buggy, and I think it's the reason for the later
bugs that Emmanuel reported.
In particular, the *non-error* case is buggy, where it in the middle
of the function does
return ipcp;
for a successful lookup.
It used to return a locked ipcp, now it no longer does. And you didn't
change any of the callers, which still do the ipc_unlock() at the
end. So all the locking gets completely confused.
Oops.
Linus
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On 03/01/2013 11:32 PM, Michel Lespinasse wrote: I think it may be nicer to take the rcu read lock at the call site rather than in ipc_obtain_object(), to make the rcu read lock/unlock sites pair up more nicely. Either that or make an inline ipc_release_object function that pairs up with ipc_obtain_object() and just does an rcu_read_unlock(). I started on a patch series to untangle the IPC locking, so it will be a little more readable, and easier to maintain. It is a slower approach than Davidlohr's, as in, it will take a little longer to put a patch series together, but I hope it will be easier to debug... I hope to post a first iteration of the series by the middle of next week. -- All rights reversed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, 2013-03-02 at 13:24 -0800, Linus Torvalds wrote:
On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso davidlohr.bu...@hp.com
wrote:
@@ -784,7 +806,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
ipc_namespace *ns,
int err;
down_write(ids-rw_mutex);
- ipcp = ipc_lock_check(ids, id);
+ ipcp = ipc_obtain_object_check(ids, id);
if (IS_ERR(ipcp)) {
err = PTR_ERR(ipcp);
goto out_up;
@@ -801,7 +823,7 @@ struct kern_ipc_perm *ipcctl_pre_down(struct
ipc_namespace *ns,
return ipcp;
err = -EPERM;
- ipc_unlock(ipcp);
+ rcu_read_unlock();
out_up:
up_write(ids-rw_mutex);
return ERR_PTR(err);
Uhhuh. This is very buggy, and I think it's the reason for the later
bugs that Emmanuel reported.
Yes, quite buggy. I was able to mess up three different machines with
this, and since semaphores aren't the only users of ipcctl_pre_down(),
it could explain the sys_shmctl() call in the trace Emmanuel reported.
In particular, the *non-error* case is buggy, where it in the middle
of the function does
return ipcp;
for a successful lookup.
It used to return a locked ipcp, now it no longer does. And you didn't
change any of the callers, which still do the ipc_unlock() at the
end. So all the locking gets completely confused.
After updating the callers, [msgctl, semctl, shmctl]_down, to acquire
the lock for IPC_RMID and IPC_SET commands, I'm no longer seeing these
issues - so far on my regular laptop and two big boxes running my Oracle
benchmarks for a few hours. Something like below (yes, I will address
the open coded spin_lock calls):
@@ -1101,16 +1138,20 @@ static int semctl_down(struct ipc_namespace *ns, int
semid,
switch(cmd){
case IPC_RMID:
+ spin_lock(sma-sem_perm.lock);
freeary(ns, ipcp);
goto out_up;
case IPC_SET:
+ spin_lock(sma-sem_perm.lock);
err = ipc_update_perm(semid64.sem_perm, ipcp);
if (err)
goto out_unlock;
sma-sem_ctime = get_seconds();
break;
default:
+ rcu_read_unlock();
err = -EINVAL;
+ goto out_up;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, 2013-03-02 at 21:18 -0500, Rik van Riel wrote: On 03/01/2013 11:32 PM, Michel Lespinasse wrote: I think it may be nicer to take the rcu read lock at the call site rather than in ipc_obtain_object(), to make the rcu read lock/unlock sites pair up more nicely. Either that or make an inline ipc_release_object function that pairs up with ipc_obtain_object() and just does an rcu_read_unlock(). I started on a patch series to untangle the IPC locking, so it will be a little more readable, and easier to maintain. It is a slower approach than Davidlohr's, as in, it will take a little longer to put a patch series together, but I hope it will be easier to debug... I hope to post a first iteration of the series by the middle of next week. Goody, I'll be watching out for it. I have a big box rt user who uses semaphores in their very tight constraint application. While they're using them carefully, I saw a trace where contention cost vs jitter budget was a bit too high for comfort, and semctl/semop collision. -Mike -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, Mar 2, 2013 at 9:32 PM, Davidlohr Bueso davidlohr.bu...@hp.com wrote: After updating the callers, [msgctl, semctl, shmctl]_down, to acquire the lock for IPC_RMID and IPC_SET commands, I'm no longer seeing these issues - so far on my regular laptop and two big boxes running my Oracle benchmarks for a few hours. Something like below (yes, I will address the open coded spin_lock calls): Actually, please do me a favor, and do *not* change the semantics of these helper calls without changing the name of the helper. So I'd ask that instead of changing the callers, do this: - make the version that does *not* lock be called ipcctl_pre_down_nolock() - then, keep the old name, and just make it do the ipcctl_pre_down_nolock() followed by the spin_lock() (or rather, the non-open-coded one). Then, you can make each caller individually use the nolock version instead, and move the locking into the caller. But don't do the same mistake as the original patch, which was to change the locking semantics while keeping the same name of the function. In other words, it's much better to make these changes in small gradual pieces, and make each piece very obviously not change any semantics. So the first piece is the introduce the helper functions with new semantics (and new names), while keeping the old helpers with the old semantics and old names. Make it really clear that no semantics change. And the second piece is then to start using the split-up helpers that have different locking semantics and new names, and do it on a call-by-call basis. Ok? Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, Mar 2, 2013 at 8:16 AM, Davidlohr Bueso wrote:
> Through ipc_lock() and, therefore, ipc_lock_check() we currently
> return the locked ipc object. This is not necessary for all situations,
> thus introduce, analogous, ipc_obtain_object and ipc_obtain_object_check
> functions that only mark the RCU read critical region without acquiring
> the lock and return the ipc object.
>
> Signed-off-by: Davidlohr Bueso
> ---
> ipc/util.c | 42 --
> ipc/util.h | 2 ++
> 2 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/ipc/util.c b/ipc/util.c
> index 464a8ab..902f282 100644
> --- a/ipc/util.c
> +++ b/ipc/util.c
> @@ -667,6 +667,21 @@ void ipc64_perm_to_ipc_perm (struct ipc64_perm *in,
> struct ipc_perm *out)
> out->seq= in->seq;
> }
>
> +struct kern_ipc_perm *ipc_obtain_object(struct ipc_ids *ids, int id)
> +{
> + struct kern_ipc_perm *out;
> + int lid = ipcid_to_idx(id);
> +
> + rcu_read_lock();
> + out = idr_find(>ipcs_idr, lid);
> + if (!out) {
> + rcu_read_unlock();
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return out;
> +}
I think it may be nicer to take the rcu read lock at the call site
rather than in ipc_obtain_object(), to make the rcu read lock/unlock
sites pair up more nicely. Either that or make an inline
ipc_release_object function that pairs up with ipc_obtain_object() and
just does an rcu_read_unlock().
> +
> /**
> * ipc_lock - Lock an ipc structure without rw_mutex held
> * @ids: IPC identifier set
> @@ -679,18 +694,13 @@ void ipc64_perm_to_ipc_perm (struct ipc64_perm *in,
> struct ipc_perm *out)
>
> struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
> {
> - struct kern_ipc_perm *out;
> - int lid = ipcid_to_idx(id);
> + struct kern_ipc_perm *out = ipc_obtain_object(ids, id);
>
> - rcu_read_lock();
> - out = idr_find(>ipcs_idr, lid);
> - if (out == NULL) {
> - rcu_read_unlock();
> + if (!out)
I think this should be if (IS_ERR(out)) ?
Looks great otherwise.
Acked-by: Michel Lespinasse
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso wrote: > > +struct kern_ipc_perm *ipc_obtain_object(struct ipc_ids *ids, int id) This looks good.. > +struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id) The comment on ipc_checkid() says that it should only be called with the lock held. Which seems entirely bogus, and I think it's just stale. It's just checking the same ipc->seq that is only set at allocation, so it won't change, afaik. So I *think* the above is ok, but I'd want people to look at that comment and remove it if it is stale. And if it's not stale, think about this. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Fri, Mar 1, 2013 at 4:16 PM, Davidlohr Bueso davidlohr.bu...@hp.com wrote: +struct kern_ipc_perm *ipc_obtain_object(struct ipc_ids *ids, int id) This looks good.. +struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id) The comment on ipc_checkid() says that it should only be called with the lock held. Which seems entirely bogus, and I think it's just stale. It's just checking the same ipc-seq that is only set at allocation, so it won't change, afaik. So I *think* the above is ok, but I'd want people to look at that comment and remove it if it is stale. And if it's not stale, think about this. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/2] ipc: introduce obtaining a lockless ipc object
On Sat, Mar 2, 2013 at 8:16 AM, Davidlohr Bueso davidlohr.bu...@hp.com wrote:
Through ipc_lock() and, therefore, ipc_lock_check() we currently
return the locked ipc object. This is not necessary for all situations,
thus introduce, analogous, ipc_obtain_object and ipc_obtain_object_check
functions that only mark the RCU read critical region without acquiring
the lock and return the ipc object.
Signed-off-by: Davidlohr Bueso davidlohr.bu...@hp.com
---
ipc/util.c | 42 --
ipc/util.h | 2 ++
2 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/ipc/util.c b/ipc/util.c
index 464a8ab..902f282 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -667,6 +667,21 @@ void ipc64_perm_to_ipc_perm (struct ipc64_perm *in,
struct ipc_perm *out)
out-seq= in-seq;
}
+struct kern_ipc_perm *ipc_obtain_object(struct ipc_ids *ids, int id)
+{
+ struct kern_ipc_perm *out;
+ int lid = ipcid_to_idx(id);
+
+ rcu_read_lock();
+ out = idr_find(ids-ipcs_idr, lid);
+ if (!out) {
+ rcu_read_unlock();
+ return ERR_PTR(-EINVAL);
+ }
+
+ return out;
+}
I think it may be nicer to take the rcu read lock at the call site
rather than in ipc_obtain_object(), to make the rcu read lock/unlock
sites pair up more nicely. Either that or make an inline
ipc_release_object function that pairs up with ipc_obtain_object() and
just does an rcu_read_unlock().
+
/**
* ipc_lock - Lock an ipc structure without rw_mutex held
* @ids: IPC identifier set
@@ -679,18 +694,13 @@ void ipc64_perm_to_ipc_perm (struct ipc64_perm *in,
struct ipc_perm *out)
struct kern_ipc_perm *ipc_lock(struct ipc_ids *ids, int id)
{
- struct kern_ipc_perm *out;
- int lid = ipcid_to_idx(id);
+ struct kern_ipc_perm *out = ipc_obtain_object(ids, id);
- rcu_read_lock();
- out = idr_find(ids-ipcs_idr, lid);
- if (out == NULL) {
- rcu_read_unlock();
+ if (!out)
I think this should be if (IS_ERR(out)) ?
Looks great otherwise.
Acked-by: Michel Lespinasse wal...@google.com
--
Michel Walken Lespinasse
A program is never fully debugged until the last user dies.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
