Messages in this thread | | | Subject | Re: [PATCH 1/5] ipc/sem: do not call wake_sem_queue_do() prematurely | From | Manfred Spraul <> | Date | Tue, 13 Sep 2016 06:17:43 +0200 |
| |
Hi Davidlohr,
On 09/12/2016 01:53 PM, Davidlohr Bueso wrote: > ... as this call should obviously be paired with its _prepare() > counterpart. At least whenever possible, as there is no harm in > calling it bogusly as we do now in a few places. I would define the interface differently: WAKE_Q creates an initialized wake queue. There is no need to track if any tasks were added to the wake queue, it is safe to call wake_up_q(). So especially for error paths, there is no need to optimize out calls to wake_up_q() > Immediate error > semop(2) paths that are far from ever having the task block can > be simplified and avoid a few unnecessary loads on their way out > of the call as it is not deeply nested. > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> > --- > ipc/sem.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index 5e318c5f749d..a4e8bb2fae38 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -1887,16 +1887,22 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, > } > > error = -EFBIG; > - if (max >= sma->sem_nsems) > - goto out_rcu_wakeup; > + if (max >= sma->sem_nsems) { > + rcu_read_unlock(); > + goto out_free; > + } > > error = -EACCES; > - if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO)) > - goto out_rcu_wakeup; > + if (ipcperms(ns, &sma->sem_perm, alter ? S_IWUGO : S_IRUGO)) { > + rcu_read_unlock(); > + goto out_free; > + } > Is this really better/simpler? You replace "if (error) goto cleanup" with "if (error) {cleanup_1(); goto cleanup_2()}".
From my point of view, this just increases the risks that some cleanup steps are forgotten.
-- Manfred
| |