From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753289AbcGDIAq (ORCPT ); Mon, 4 Jul 2016 04:00:46 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:36238 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752844AbcGDIAn (ORCPT ); Mon, 4 Jul 2016 04:00:43 -0400 Date: Mon, 4 Jul 2016 09:00:38 +0100 From: Chris Wilson To: akash.goel@intel.com Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, viro@zeniv.linux.org.uk, Eduard - Gabriel Munteanu , Tom Zanussi Subject: Re: [PATCH] relay: Add global mode support for buffer-only channels Message-ID: <20160704080038.GA25596@nuc-i3427.alporthouse.com> References: <1467562527-21569-1-git-send-email-akash.goel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1467562527-21569-1-git-send-email-akash.goel@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 */ > + 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 > + 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)) ? > + err = -EINVAL; > + else > + relay_set_buf_dentry(chan->buf[0], dentry); > + > + mutex_unlock(&relay_channels_mutex); > + return err; This could be written: 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 -- Chris Wilson, Intel Open Source Technology Centre