linux-kernel - Re: [PATCHSET] printk: implement printk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <1200965291.3860.38.camel@cinder.waste.org>
Date:	Mon, 21 Jan 2008 19:28:11 -0600
From:	Matt Mackall <mpm@...enic.com>
To:	Tejun Heo <htejun@...il.com>
Cc:	linux-kernel@...r.kernel.org, daniel.ritz-ml@...ssonline.ch,
	randy.dunlap@...cle.com, jeff@...zik.org,
	linux-ide@...r.kernel.org, Matthew Wilcox <matthew@....cx>
Subject: Re: [PATCHSET] printk: implement printk_header() and merging printk


On Tue, 2008-01-22 at 10:00 +0900, Tejun Heo wrote:
> Matt Mackall wrote:
> > I suppose. I still find this approach less than ideal, especially
> > putting something potentially large on the stack. The dangers are
> > perhaps worse than a malloc, really. 
> 
> I pondered on this a bit but the thing is we already use several
> hundreds bytes in a function which builds complex messages.

Well there are lots of current and potential future users of this
function, many of them down at the end of long call chains. So I'm more
worried about the new cases that embrace this approach and suddenly add
300 bytes of stack. In fact, if this is at all popular, we can expect to
have more than one of these frames on the stack in various paths. 

Given that it only exists to make output prettier, it doesn't -really-
justify increased stack usage.

> > I also don't like your interface much. Consider this alternative:
> > 
> > struct mprintk *mp = mprintk_begin(KERN_INFO "ata%u.%2u: ", 1, 0);
> > mprintk(mp, "ATA %d", 7);
> > mprintk(mp, ", %u sectors\n", 1024);
> > mprintk(mp, "everything seems dandy\n");
> > mprintk_end(mp);
> > 
> > That keeps all the "normal" printks short and makes the flush more
> > explict.
> 
> I like that the more used function is shorter.  Hmmm... The reason why I
> first used mprintk_push() is to make it clear that the function
> accumulates messages unlike mprintk() which flushes what's accumulated
> and prints its own message.
> 
> > Now we make mprintk_begin attempt to do a kmalloc of a moderate size
> > (512 bytes?) and failing that, return null. Then mprintk can fall
> > through to printk in the NULL case.
> 
> If you wanna do that implicitly, you need GFP_ flag in mprintk_begin()
> and atomic allocation should be used from interrupt handlers and friends
> and they fail easily under the right (or wrong) conditions.  Forcing
> kmalloc isn't a good idea.  Having multiple initializers is one way to
> do it.  Any suggestions?

Adding a gfp_flags arg isn't too painful. And we've generally avoided
having separate function calls for atomic vs non-atomic allocation.

Btw, we can also easily hide Willy or Rusty's stringbuf implementation
under the covers here and still have a scheme that automatically falls
back to direct printk..

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/