From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758760AbcIRS07 (ORCPT ); Sun, 18 Sep 2016 14:26:59 -0400 Received: from mx2.suse.de ([195.135.220.15]:33717 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754653AbcIRS0v (ORCPT ); Sun, 18 Sep 2016 14:26:51 -0400 Date: Sun, 18 Sep 2016 11:26:46 -0700 From: Davidlohr Bueso To: Manfred Spraul Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Davidlohr Bueso Subject: Re: [PATCH 2/5] ipc/sem: rework task wakeups Message-ID: <20160918182646.GA22474@linux-80c1.suse> References: <1473681216-20025-1-git-send-email-dave@stgolabs.net> <1473681216-20025-3-git-send-email-dave@stgolabs.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 18 Sep 2016, Manfred Spraul wrote: >>+ <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Why this empty line? That's my fat fingers, will remove it. >>+ } >>+ >>+ sem_unlock(sma, locknum); >>+ rcu_read_unlock(); >>+ wake_up_q(&wake_q); >>+ >>+ goto out_free; >> } >>- if (error <= 0) >>- goto out_unlock_free; >I don't see the strategy: >I've used the approach that cleanup is at the end, to reduce >duplicated code, even if it means that error codepaths unnecessarily >call wakeup for an empty list and that the list is always initialized. > >With patch 1 of the series, you start to optimize for that. >Now this patch reintroduces some wake_up_q calls for error paths. Well yes, but this is a much more self contained than what we currently have in that at least perform_atomic_semop() was called. Yes, an error path will still call wake_up_q unnecessarily, but its pretty obvious what's going on within that error <= 0 condition. I really don't think this is a big deal. In addition the general exit path of the function is also slightly cleaned up as a consequence. >So: What is the aim? >I would propose to skip patch 1 and leave the wake_up_q at the end. > >Or, if we really want to avoid the wakeup calls, then do it entirely. >Perhaps: >> if(error == 0) { /* nonblocking codepath 1, with wakeups */ >> [...] >> } >> if (error < 0} goto out_unlock_free; >> >This would have an advantage, because the WAKE_Q would be initialized >only when needed Sure. Note that we can even get picky with this in semctl calls, but I'm ok with some unnecessary initialization and wake_up_q paths. Please shout if you really want me to change them and I can add followup patches, although I suspect you'll agree. Thanks, Davidlohr