From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753889AbcGDNDV (ORCPT ); Mon, 4 Jul 2016 09:03:21 -0400 Received: from mga09.intel.com ([134.134.136.24]:53076 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753836AbcGDNDN (ORCPT ); Mon, 4 Jul 2016 09:03:13 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,574,1459839600"; d="scan'208";a="1000559215" Subject: Re: [PATCH] relay: Add global mode support for buffer-only channels To: Chris Wilson References: <1467562527-21569-1-git-send-email-akash.goel@intel.com> <20160704080038.GA25596@nuc-i3427.alporthouse.com> Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, viro@zeniv.linux.org.uk, Eduard - Gabriel Munteanu , Tom Zanussi , akash.goel@intel.com From: "Goel, Akash" Message-ID: Date: Mon, 4 Jul 2016 18:33:08 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160704080038.GA25596@nuc-i3427.alporthouse.com> 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 7/4/2016 1:30 PM, Chris Wilson wrote: > On Sun, Jul 03, 2016 at 09:45:27PM +0530, akash.goel@intel.com wrote: >> From: Akash Goel >> >> The following patch added support to use channels with no associated files. >> relay: add buffer-only channels; useful for early logging >> This is useful when the exact location of relay file is not known or the >> the parent directory of relay file is not available, while creating the >> channel and the logging has to start right from the boot. >> >> But there was no provision to use global mode with buffer-only channels, >> which is added by this patch, without modifying the interface where initially >> there will be a dummy invocation of create_buf_file callback through which >> kernel client can convey the need of a global buffer. >> >> For the use case where drivers/kernel clients want a simple interface for the >> userspace, which enables them to capture data/logs from relay file in order & >> without any post processing, support of Global buffer mode is warranted. >> >> Cc: Eduard - Gabriel Munteanu >> Cc: Tom Zanussi >> Cc: Chris Wilson >> Signed-off-by: Akash Goel >> --- >> kernel/relay.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/kernel/relay.c b/kernel/relay.c >> index 04d7cf3..b267384 100644 >> --- a/kernel/relay.c >> +++ b/kernel/relay.c >> @@ -451,6 +451,16 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu) >> if (!dentry) >> goto free_buf; >> relay_set_buf_dentry(buf, dentry); >> + } else { > > The trailing block can be replaced with: > > /* Only retrieve global info, nothing more, nothing less */ > Thanks, will do like this. >> + dentry = chan->cb->create_buf_file(NULL, NULL, >> + S_IRUSR, buf, >> + &chan->is_global); > > Watch alignment, align to the opening bracket. > >> + if (WARN_ON(dentry)) >> + goto free_buf; >> } >> >> buf->cpu = cpu; >> @@ -666,6 +676,27 @@ int relay_late_setup_files(struct rchan *chan, >> } >> chan->has_base_filename = 1; >> chan->parent = parent; >> + >> + if (chan->is_global) { >> + if (unlikely(!chan->buf[0])) { > > if (WARN_ON_ONCE(!chan->buf[0])) { > >> + WARN_ONCE(1, KERN_ERR "CPU 0 has no buffer!\n"); > > The WARN includes the unlikely and the message here isn't any more > informative than the line itself, plus has the erroneous KERN_ERR > Thanks will combine the WARN and if check. >> + mutex_unlock(&relay_channels_mutex); >> + return -EINVAL; >> + } >> + >> + dentry = relay_create_buf_file(chan, chan->buf[0], 0); >> + >> + if (unlikely(!dentry)) >> + err = -EINVAL; >> + else if (WARN_ON(!chan->is_global)) > > ? > Though have checked 'is_global' value before, but can't rule out the clients toggling its value (inadvertently or maliciously). Actually create_buf_file() callback will be invoked again from the relay_create_buf_file() and pointer of 'chan->is_global' is passed in that. So to be on safer side, thought to validate the 'chan->is_global' value after the call to relay_create_buf_file(). >> + err = -EINVAL; >> + else >> + relay_set_buf_dentry(chan->buf[0], dentry); >> + >> + mutex_unlock(&relay_channels_mutex); >> + return err; > > This could be written: > Thanks this is much better. Best Regards Akash > if (chan->is_global) { > err = -EINVAL; > if (!WARN_ON_ONCE(!chan->buf[0])) { > dentry = relay_create_buf_file(chan, chan->buf[0], 0)) > if (dentry) { > relay_set_buf_dentry(chan->buf[0], dentry); > err = 0; > } > } > mutex_unlock(); > return err; > } > > for a single exit/unlock path in this block. > -Chris >