From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932710AbcISSlD (ORCPT ); Mon, 19 Sep 2016 14:41:03 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36850 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932536AbcISSlB (ORCPT ); Mon, 19 Sep 2016 14:41:01 -0400 Subject: Re: [PATCH -next v2 0/5] ipc/sem: semop(2) improvements To: Davidlohr Bueso , akpm@linux-foundation.org References: <1474225896-10066-1-git-send-email-dave@stgolabs.net> Cc: linux-kernel@vger.kernel.org From: Manfred Spraul Message-ID: <50042b05-2e9c-8483-710c-0f0eafc658e0@colorfullife.com> Date: Mon, 19 Sep 2016 20:40:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <1474225896-10066-1-git-send-email-dave@stgolabs.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/18/2016 09:11 PM, Davidlohr Bueso wrote: > Changes from v1 (https://lkml.org/lkml/2016/9/12/266) > - Got rid of the signal_pending check in wakeup fastpath. (patch 2) > - Added read/access once to queue.status (we're obviously concerned about > lockless access upon unrelated events, even if on the stack). > - Got rid of initializing wake_q and wake_up_q call upon perform_atomic_semop > error return path. (patch 2) > - Documented ordering between wake_q_add and setting ->status. > - What I did not do was refactor the checks in perfor_atomic_semop[_slow] > as I could not get a decent/clean way of doing it without adding more > unnecessary code. If we wanted to do smart semop scans that we received from > userspace, this would still need to be done under sem_lock for semval values > obviously. So I've left it as is, where we mainly duplicate the function, but > I still believe this is the most straightforward way of dealing with this > situation (patch 3). > - Replaced using SEMOP_FAST with BITS_PER_LONG, as this is really what we want > to limit the duplicate scanning. > - More testing. > - Added Manfred's ack (patch 5). > > Hi, > > Here are a few updates around the semop syscall handling that I noticed while > reviewing Manfred's simple vs complex ops fixes. Changes are on top of -next, > which means that Manfred's pending patches to ipc/sem.c that remove the redundant > barrier(s) would probably have to be rebased. > > The patchset has survived the following testscases: > - ltp > - ipcsemtest (https://github.com/manfred-colorfu/ipcsemtest) > - ipcscale (https://github.com/manfred-colorfu/ipcscale) > > Details are in each individual patch. Please consider for v4.9. > > Thanks! > > Davidlohr Bueso (5): > ipc/sem: do not call wake_sem_queue_do() prematurely The only patch that I don't like. Especially: patch 2 of the series removes the wake_up_q from the function epilogue. So only the code duplication (additional instances of rcu_read_unlock()) remains, I don't see any advantages. > ipc/sem: rework task wakeups Acked > ipc/sem: optimize perform_atomic_semop() I'm still thinking about it. Code duplication is evil, but perhaps it is the best solution. What I don't like is the hardcoded "< BITS_PER_LONG". At least: - (1 << sop->sem_num) + (1 << (sop->sem_num%BITS_PER_LONG)) > ipc/sem: explicitly inline check_restart Do we really need that? Isn't that the compiler's task? Especially since the compiler is already doing it correctly. > ipc/sem: use proper list api for pending_list wakeups Acked > ipc/sem.c | 415 ++++++++++++++++++++++++++++++-------------------------------- > 1 file changed, 199 insertions(+), 216 deletions(-) > -- Manfred