Re: block: make blktrace use per-cpu buffers for message notes

[Jens Axboe <jens.axboe@oracle.com>]

On Wed, May 28 2008, Andrew Morton wrote:
> On Wed, 28 May 2008 15:59:07 GMT Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
> 
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=64565911cdb57c2f512a9715b985b5617402cc67
> > Commit:     64565911cdb57c2f512a9715b985b5617402cc67
> > Parent:     4722dc52a891ab6cb2d637ddb87233e0ce277827
> > Author:     Jens Axboe <jens.axboe@oracle.com>
> > AuthorDate: Wed May 28 14:45:33 2008 +0200
> > Committer:  Jens Axboe <jens.axboe@oracle.com>
> > CommitDate: Wed May 28 14:49:27 2008 +0200
> 
> please try to avoid merging unreviewed changes.

Just because you didn't review it doesn't mean it's unreviewed :-)

It's not unreviewed, it was posted on lkml and a few version were
bounced back and forth.

> >     block: make blktrace use per-cpu buffers for message notes
> >     
> >     Currently it uses a single static char array, but that risks
> >     being corrupted when multiple users issue message notes at the
> >     same time. Make the buffers dynamically allocated when the trace
> >     is setup and make them per-cpu instead.
> >     
> >     The default max message size of 1k is also very large, the
> >     interface is mainly for small text notes. So shrink it to 128 bytes.
> >     
> >     Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > ---
> >  block/blktrace.c             |   15 ++++++++++++---
> >  include/linux/blktrace_api.h |    3 ++-
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/blktrace.c b/block/blktrace.c
> > index 20e11f3..7ae87cc 100644
> > --- a/block/blktrace.c
> > +++ b/block/blktrace.c
> > @@ -79,13 +79,16 @@ void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
> >  {
> >  	int n;
> >  	va_list args;
> > -	static char bt_msg_buf[BLK_TN_MAX_MSG];
> > +	char *buf;
> >  
> > +	preempt_disable();
> > +	buf = per_cpu_ptr(bt->msg_data, smp_processor_id());
> 
> get_cpu()/put_cpu() is the standard way of doing this.

Sure, end result is the same though. get_cpu()/put_cpu() is cleaner,
though.

> > @@ -172,7 +173,7 @@ extern void __trace_note_message(struct blk_trace *, const char *fmt, ...);
> >  		if (unlikely(bt))					\
> >  			__trace_note_message(bt, fmt, ##__VA_ARGS__);	\
> >  	} while (0)
> > -#define BLK_TN_MAX_MSG		1024
> > +#define BLK_TN_MAX_MSG		128
> 
> It seems a bit strange to do this right when we've taken this _off_ the
> stack.  But I suppose nothing will break.

It was never on the stack, it was a global static char array. We are
still allocating memory for this, per-cpu. So I think it still makes
sense to shrink the size. It's really meant for small trace messages,
128 bytes is plenty. It's an in-kernel property, the userland app
doesn't care. So we could easily grow this in the future, should the
need arise.

> I cannot work out why 9d5f09a424a67ddb959829894efb4c71cbf6d600 ("Added
> in MESSAGE notes for blktraces") was -rc material.

Well it's probably really not, but the impact is truly minimal. But
strictly speaking, it was not -rcX material, I agree.

-- 
Jens Axboe