Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: Andrew Morton wrote: So, I guess it's NACK w/o suggested alternatives, right? I wouldn't nack without good reasons, and I have none here. I don't have very strong opinions either way. I was just wondering whether I should just go with snprintf dancing in eh_link_report, which does make sense if not many need merging printk. .. Any chance you could poke through snprintf() and look for the off-by-one bug on the return result? (I think it happens when "n" is exceeded). :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >> So, I guess it's NACK w/o suggested alternatives, right? > > I wouldn't nack without good reasons, and I have none here. I don't have > very strong opinions either way. I was just wondering whether I should just go with snprintf dancing in eh_link_report, which does make sense if not many need merging printk. > As a seat-of-the-pants thing, it does seem to be a lot of core code to > solve a fairly minor problem in (afaik) one remote place. But I haven't > looked - perhaps there are other places which could be improved if such > facilities were available. Okay, I see. I'll look around and see whether there are other places which can use it. I can think of a few in SCSI. Let's see. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Fri, 15 Feb 2008 11:36:12 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > Andrew Morton wrote: > >>> printk is a special case, I think. It's the primary logging/debugging > >>> method which can't fail and as it's mostly interpreted by human beings > >>> (and developers in problematic cases), it has different maneuvering room > >>> on errors - ie. it's far better to print messages w/o header or proper > >>> log level than failing to print, which is quite different requirements > >>> from other components. > >> Andrew, any more comments or suggestions on how to proceed on this? > > > > Nope. > > > >> One > >> way or the other, I think this is a problem worth solving. > > > > There are a lot of such problems ;) > > So, I guess it's NACK w/o suggested alternatives, right? > I wouldn't nack without good reasons, and I have none here. I don't have very strong opinions either way. As a seat-of-the-pants thing, it does seem to be a lot of core code to solve a fairly minor problem in (afaik) one remote place. But I haven't looked - perhaps there are other places which could be improved if such facilities were available. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >>> printk is a special case, I think. It's the primary logging/debugging >>> method which can't fail and as it's mostly interpreted by human beings >>> (and developers in problematic cases), it has different maneuvering room >>> on errors - ie. it's far better to print messages w/o header or proper >>> log level than failing to print, which is quite different requirements >>> from other components. >> Andrew, any more comments or suggestions on how to proceed on this? > > Nope. > >> One >> way or the other, I think this is a problem worth solving. > > There are a lot of such problems ;) So, I guess it's NACK w/o suggested alternatives, right? -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Fri, 15 Feb 2008 10:49:31 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > > printk is a special case, I think. It's the primary logging/debugging > > method which can't fail and as it's mostly interpreted by human beings > > (and developers in problematic cases), it has different maneuvering room > > on errors - ie. it's far better to print messages w/o header or proper > > log level than failing to print, which is quite different requirements > > from other components. > > Andrew, any more comments or suggestions on how to proceed on this? Nope. > One > way or the other, I think this is a problem worth solving. There are a lot of such problems ;) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: > Hello, > > Andrew Morton wrote: >> On Thu, 14 Feb 2008 09:40:51 +0900 >> Tejun Heo <[EMAIL PROTECTED]> wrote: >> >>> Can you please take a look at ata_eh_link_report() in >>> drivers/ata/libata-eh.c? >> I did. Punishment? > > Heh.. :-) > >>> Currently, it has some problems. >> Yes, and the patches do clean that up. > > Yeap, it does. > >> ho hum. What tends to happen with this sort of thing is that fi we merge >> it, it ends up getting used more often than one expected... > > Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over > usual expectations but well, yeah, that actually is my expectation. > >> If you stand back and squint at it, there are quite a few places where we >> do this sort of thing: allocate a buffer, squirt characters into it, >> reallocating and/or flushing as we proceed. All sysfs and procfs read-side >> code, for a start... > > printk is a special case, I think. It's the primary logging/debugging > method which can't fail and as it's mostly interpreted by human beings > (and developers in problematic cases), it has different maneuvering room > on errors - ie. it's far better to print messages w/o header or proper > log level than failing to print, which is quite different requirements > from other components. Andrew, any more comments or suggestions on how to proceed on this? One way or the other, I think this is a problem worth solving. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: .. * For timeouts, result TF isn't available and thus res printout is misleading. res shouldn't be printed after timeouts. This would require allocating yest another temp buf and separating out res printing into separate snprintf. .. And snprintf() is buggy, by the way. It does not always seem to return the correct character fill counts. I've given up trying to use it here for anything in kernelspace that *must* work. Someday I'll go back and try to figure out where it's screwing up. I think it is doing so in the cases where it runs out of space in the buffer. -ml -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Hello, Andrew Morton wrote: > On Thu, 14 Feb 2008 09:40:51 +0900 > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> Can you please take a look at ata_eh_link_report() in >> drivers/ata/libata-eh.c? > > I did. Punishment? Heh.. :-) >> Currently, it has some problems. > > Yes, and the patches do clean that up. Yeap, it does. > ho hum. What tends to happen with this sort of thing is that fi we merge > it, it ends up getting used more often than one expected... Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over usual expectations but well, yeah, that actually is my expectation. > If you stand back and squint at it, there are quite a few places where we > do this sort of thing: allocate a buffer, squirt characters into it, > reallocating and/or flushing as we proceed. All sysfs and procfs read-side > code, for a start... printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Thu, 14 Feb 2008 09:40:51 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > Can you please take a look at ata_eh_link_report() in > drivers/ata/libata-eh.c? I did. Punishment? > Currently, it has some problems. Yes, and the patches do clean that up. ho hum. What tends to happen with this sort of thing is that fi we merge it, it ends up getting used more often than one expected... If you stand back and squint at it, there are quite a few places where we do this sort of thing: allocate a buffer, squirt characters into it, reallocating and/or flushing as we proceed. All sysfs and procfs read-side code, for a start... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >> And mprintk the following. >> >> code: >> DEFINE_MPRINTK(mp, 2 * 80); >> >> mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); >> mprintk_push(&mp, "ATA %d", 7); >> mprintk_push(&mp, ", %u sectors\n", 1024); >> mprintk(&mp, "everything seems dandy\n"); >> >> output: >> <6>ata1.00: ATA 7, 1024 sectors >> <6>ata1.00 everything seems dandy > > And that looks like an awful lot of fuss. Is it really worth doing? In the above example, s/mprintk(/mprintk_flush(/ and s/mprintk_push(/mprintk(/ Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? Currently, it has some problems. * All that it wants to do is printing some messages but it's awfully complex with temp bufs and super-long printk calls containing optional %s's. * status / error decode print outs are printed separately from cmd/res making it difficult to tell how they are grouped. Putting them together would require allocating another temp buf(s) and adding extra %s's to cmd/res printout. * For timeouts, result TF isn't available and thus res printout is misleading. res shouldn't be printed after timeouts. This would require allocating yest another temp buf and separating out res printing into separate snprintf. I was trying to do this and got fed up with all the tricks in the function. The only sane way out is to build messages piece-by-piece into a buffer and print it at once. The eh message is gigantic and I needed to allocate the buffer elsewhere than stack. ata_eh_link_report() fortunately has fixed place for that - ap->sector_buf - but let's assume there was no such buffer for the discussion. I'm still not too sure whether it's wise to use sector_buf for message building anyway. The only way is to malloc the buffer. Once the buffer is available, building message using snprintf is a bit cumbersome but is okay. The problem is that malloc can fail. To handle that case, we basically need to do if (buf) printed += snprintf(buf + printed, len - printed, ...); else printk(...); which is very cumbersome, so we need a wrapper around the above. As the wrapper needs to control when the message goes out, a flush function is necessary too. Combine those with overflow handling - mprintk. Similar problem exists for ata_dev_configure() in drivers/ata/libata-core.c although it's a bit better there. Please take a look at the fifth patch. It doesn't remove a lot of lines but it streamlines both functions significantly. For ata_dev_configure(), message reporting becomes what the function does secondarily while configuring the device, not something it's structured around. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Wed, 13 Feb 2008 18:09:28 +0900 Tejun Heo <[EMAIL PROTECTED]> wrote: > This is the third take of implement-printk_header-and-mprintk > patchset. > > Changes from the last take[L] are... > > * Now header is printed on every line of a multiline message. If the > header ends with ':' followed by spaces. The colon is replaced with > space from the second line. > > * s/mprintk/mprintk_flush/ and s/mprintk_add/mprintk/ as suggested. > > * mprintk_init_alloc() and mprintk_free() added to ease malloc'd > buffer handling. No need to specify buffer size. Sizing is left to > mprintk. > > This patchset aims to make printing multiline messages and assembling > message piece-by-piece easier. > > In a nutshell, printk_header() lets you do the following atomically > (against other messages). > > code: > printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); > > output: > <6>ata1.00: line0 > <6>ata1.00 line1 > <6>ata1.00 line2 That seems sensible. > And mprintk the following. > > code: > DEFINE_MPRINTK(mp, 2 * 80); > > mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > mprintk_push(&mp, "ATA %d", 7); > mprintk_push(&mp, ", %u sectors\n", 1024); > mprintk(&mp, "everything seems dandy\n"); > > output: > <6>ata1.00: ATA 7, 1024 sectors > <6>ata1.00 everything seems dandy > And that looks like an awful lot of fuss. Is it really worth doing? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHSET] printk: implement printk_header() and merging printk, take #3
Hello, all. This is the third take of implement-printk_header-and-mprintk patchset. Changes from the last take[L] are... * Now header is printed on every line of a multiline message. If the header ends with ':' followed by spaces. The colon is replaced with space from the second line. * s/mprintk/mprintk_flush/ and s/mprintk_add/mprintk/ as suggested. * mprintk_init_alloc() and mprintk_free() added to ease malloc'd buffer handling. No need to specify buffer size. Sizing is left to mprintk. This patchset aims to make printing multiline messages and assembling message piece-by-piece easier. In a nutshell, printk_header() lets you do the following atomically (against other messages). code: printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); output: <6>ata1.00: line0 <6>ata1.00 line1 <6>ata1.00 line2 And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); mprintk_push(&mp, "ATA %d", 7); mprintk_push(&mp, ", %u sectors\n", 1024); mprintk(&mp, "everything seems dandy\n"); output: <6>ata1.00: ATA 7, 1024 sectors <6>ata1.00 everything seems dandy For more info, please read Documentation/printk.txt which is created by the fourth patch. This patchset is on top of linux-2.6#master (96b5a46e2a72dc1829370c87053e0cd558d58bc0) + printk-fix-possible-printk-buffer-overrun [1] + printk-implement-printk_buf-overflow-warning [2] and contains the following patches. 0001-printk-keep-log-level-on-multiline-messages.patch 0002-printk-implement-v-printk_header.patch 0003-printk-implement-merging-printk.patch 0004-printk-add-Documentation-printk.txt.patch 0005-libata-make-libata-use-printk_header-and-mprintk.patch Documentation/00-INDEX |2 Documentation/printk.txt| 724 drivers/ata/libata-core.c | 204 +++- drivers/ata/libata-eh.c | 182 +-- drivers/ata/libata-pmp.c|5 drivers/ata/libata-scsi.c |6 drivers/ata/sata_inic162x.c |2 drivers/ata/sata_nv.c |4 include/linux/kernel.h | 85 + include/linux/libata.h | 35 +- kernel/printk.c | 435 -- 11 files changed, 1453 insertions(+), 231 deletions(-) If this gets acked, I think it's best to push it through libata-dev as libata is the first user. Thanks. -- tejun [L] http://thread.gmane.org/gmane.linux.ide/27317 [1] http://article.gmane.org/gmane.linux.kernel/639018 [2] http://article.gmane.org/gmane.linux.kernel/639029 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
Jan Engelhardt wrote: > On Jan 23 2008 08:51, Tejun Heo wrote: >> What do you think about the second suggestion then? >> >> ata1.00: line0 >> ata1.00 line1 >> ata1.00 line2 >> >> It allows you to grab for the header && has indication for message >> boundaries. > > Then again, why not "[ata1.00] line0", then it matches what sd_mod does :) Well, that's fine too but using ':' is much more common. Just take a look at the boot log and if we go with '[]', any ideas on how to indicate multiline messages? -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
On Jan 23 2008 08:51, Tejun Heo wrote: > >What do you think about the second suggestion then? > >ata1.00: line0 >ata1.00 line1 >ata1.00 line2 > >It allows you to grab for the header && has indication for message >boundaries. Then again, why not "[ata1.00] line0", then it matches what sd_mod does :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
Jan Engelhardt wrote: > On Jan 21 2008 05:48, Matthew Wilcox wrote: >> On Mon, Jan 21, 2008 at 02:13:52PM +0900, Tejun Heo wrote: >>> In a nutshell, printk_header() lets you do the following atomically >>> (against other messages). >>> >>> code: >>> printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); >>> >>> output: >>> <6>ata1.00: line0 >>> <6> line1 >>> <6> line2 >> I think this is a really bad idea. It's much better to have: >> >> <6>ata1.00: line0 >> <6>ata1.00: line1 >> <6>ata1.00: line2 >> >> That way you can grep for ata1.00 and get all messages relevant to that >> device. > > Agreed, I too prefer <6>ata1.00 on every line. What do you think about the second suggestion then? ata1.00: line0 ata1.00 line1 ata1.00 line2 It allows you to grab for the header && has indication for message boundaries. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
On Jan 21 2008 05:48, Matthew Wilcox wrote: >On Mon, Jan 21, 2008 at 02:13:52PM +0900, Tejun Heo wrote: >> In a nutshell, printk_header() lets you do the following atomically >> (against other messages). >> >> code: >> printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); >> >> output: >> <6>ata1.00: line0 >> <6> line1 >> <6> line2 > >I think this is a really bad idea. It's much better to have: > ><6>ata1.00: line0 ><6>ata1.00: line1 ><6>ata1.00: line2 > >That way you can grep for ata1.00 and get all messages relevant to that >device. Agreed, I too prefer <6>ata1.00 on every line. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
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. The original ata_eh_report() implementation allocates 424 bytes on stack for temp buffers and local variables. In addition to that, it calls printk with upto 30 arguments (~240 bytes). While the new implementation allocates 232 bytes sans the buffer and the maximum number of arguments is about sixteen (~128 bytes). ata_eh_report() uses a fixed buffer but 320byte buffer should be sufficient. In total, it's 664 vs 680 and that's for a really big message. mprintk also allows fixed or malloc'd buffers so if you wanna go bigger, malloc'd buffer should do the job. > 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? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
On Sat, 2008-01-19 at 07:58 +0900, Tejun Heo wrote: > Matt Mackall wrote: > > On Wed, 2008-01-16 at 10:00 +0900, Tejun Heo wrote: > >> And mprintk the following. > >> > >> code: > >> DEFINE_MPRINTK(mp, 2 * 80); > >> > >> mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > >> mprintk_push(&mp, "ATA %d", 7); > >> mprintk_push(&mp, ", %u sectors\n", 1024); > >> mprintk(&mp, "everything seems dandy\n"); > > > > I prefer Matthew Wilcox's stringbuf approach which does proper memory > > management and isn't specific to printk: > > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0710.3/0517.html > > Yeap, that's generic and nice but I think both 'generic' and 'proper > memory management' are weakness if what you're trying to do is to > support collecting messages in pieces and putting it out via printk. > Please consider the following scenario. > > You're in an interrupt handler and detected a severe error condition > which should be notified to the user but the information is rather > complex and best built in pieces, so you create a stringbuf and does > sb_printf() to it w/ GFP_ATOMIC but alas memory allocation failed and > you end up printing "out of memory" unless you detect the failure and go > back and printk messages piece-by-piece manually. I would rather > assemble the message manually from the get-go into an on-stack buffer. 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 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. 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. -- 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
Matthew Wilcox wrote: > On Mon, Jan 21, 2008 at 02:13:52PM +0900, Tejun Heo wrote: >> In a nutshell, printk_header() lets you do the following atomically >> (against other messages). >> >> code: >> printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); >> >> output: >> <6>ata1.00: line0 >> <6> line1 >> <6> line2 > > I think this is a really bad idea. It's much better to have: > > <6>ata1.00: line0 > <6>ata1.00: line1 > <6>ata1.00: line2 > > That way you can grep for ata1.00 and get all messages relevant to that > device. It has some pros and cons. Having indentation makes things easier on human eyes but more difficult on scripts, but not too difficult - indented lines can easily be matched with a bit of scripting. Changing the behavior is easy but I'm still inclined toward indentation because printing header every line loses information about message boundaries. Maybe there's a way to satisfy both like omitting the separator from the second line on. ata1.00: line0 ata1.00 line1 ata1.00 line2 Hmm... Any better ideas? -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #2
On Mon, Jan 21, 2008 at 02:13:52PM +0900, Tejun Heo wrote: > In a nutshell, printk_header() lets you do the following atomically > (against other messages). > > code: > printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); > > output: > <6>ata1.00: line0 > <6> line1 > <6> line2 I think this is a really bad idea. It's much better to have: <6>ata1.00: line0 <6>ata1.00: line1 <6>ata1.00: line2 That way you can grep for ata1.00 and get all messages relevant to that device. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHSET] printk: implement printk_header() and merging printk, take #2
Hello, all. This is the second take of implement-printk_header-and-mprintk patchset. Changes from the last take[L] are... * s/mprintk_push/mprintk_add/ and other changes suggested by Randy Dunlap. * mp->buf instead of mp->header to ease freeing kmalloc'd buffer later. * mprintk multiline log level handling fixed and improved. mp->prv is removed and mp->flags is used to track whether header log level exists and log level is overridden. One critical bug was re-using va_list more than once resulting in dereferences of wrong addresses. Fixed by using va_copy(). This patchset aims to make printing multiline messages and assembling message piece-by-piece easier. In a nutshell, printk_header() lets you do the following atomically (against other messages). code: printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); output: <6>ata1.00: line0 <6> line1 <6> line2 And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); mprintk_push(&mp, "ATA %d", 7); mprintk_push(&mp, ", %u sectors\n", 1024); mprintk(&mp, "everything seems dandy\n"); output: <6>ata1.00: ATA 7, 1024 sectors <6> everything seems dandy For more info, please read Documentation/printk.txt which is created by the fourth patch. This patchset is against the current linux-2.6#master (a7da60f4) and contains the following patches. 0001-printk-keep-log-level-on-multiline-messages.patch 0002-printk-implement-v-printk_header.patch 0003-printk-implement-merging-printk.patch 0004-printk-add-Documentation-printk.txt.patch 0005-libata-make-libata-use-printk_header-and-mprintk.patch Documentation/00-INDEX |2 Documentation/printk.txt| 711 drivers/ata/libata-core.c | 202 +++- drivers/ata/libata-eh.c | 150 - drivers/ata/libata-pmp.c|5 drivers/ata/libata-scsi.c |6 drivers/ata/sata_inic162x.c |2 drivers/ata/sata_nv.c |4 include/linux/kernel.h | 81 + include/linux/libata.h | 35 +- kernel/printk.c | 392 ++-- 11 files changed, 1379 insertions(+), 211 deletions(-) Randy, Jeff, if this take gets acked, how about pushing this through libata-dev as it's the first user and I have other libata changes depending on mprintk? Thanks. -- tejun [L] http://thread.gmane.org/gmane.linux.ide/27199 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
Matt Mackall wrote: > On Wed, 2008-01-16 at 10:00 +0900, Tejun Heo wrote: >> And mprintk the following. >> >> code: >> DEFINE_MPRINTK(mp, 2 * 80); >> >> mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); >> mprintk_push(&mp, "ATA %d", 7); >> mprintk_push(&mp, ", %u sectors\n", 1024); >> mprintk(&mp, "everything seems dandy\n"); > > I prefer Matthew Wilcox's stringbuf approach which does proper memory > management and isn't specific to printk: > > http://www.ussg.iu.edu/hypermail/linux/kernel/0710.3/0517.html Yeap, that's generic and nice but I think both 'generic' and 'proper memory management' are weakness if what you're trying to do is to support collecting messages in pieces and putting it out via printk. Please consider the following scenario. You're in an interrupt handler and detected a severe error condition which should be notified to the user but the information is rather complex and best built in pieces, so you create a stringbuf and does sb_printf() to it w/ GFP_ATOMIC but alas memory allocation failed and you end up printing "out of memory" unless you detect the failure and go back and printk messages piece-by-piece manually. I would rather assemble the message manually from the get-go into an on-stack buffer. By being specifially 'printk' and let the user supply buffer, which in most cases can be on-stack (messages shouldn't be too long anyway), mprintk either can avoid those problems from the beginning or can automatically work around when problem arises (initialized with NULL buffer from allocation failure) without losing any message, so it's essentially as simple as using printk. There is no error handling (both mprintk and kfree can handle NULL pointer) and the message is guaranteed to go out no matter what. Auto-expanding string buffer is nice but I don't think it fits the bill here. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
On Fri, 2008-01-18 at 11:44 -0700, Matthew Wilcox wrote: > On Fri, Jan 18, 2008 at 12:41:08PM -0600, Matt Mackall wrote: > > On Wed, 2008-01-16 at 10:00 +0900, Tejun Heo wrote: > > > And mprintk the following. > > > > > > code: > > > DEFINE_MPRINTK(mp, 2 * 80); > > > > > > mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > > > mprintk_push(&mp, "ATA %d", 7); > > > mprintk_push(&mp, ", %u sectors\n", 1024); > > > mprintk(&mp, "everything seems dandy\n"); > > > > I prefer Matthew Wilcox's stringbuf approach which does proper memory > > management and isn't specific to printk: > > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0710.3/0517.html > > Do you have any strong feelings whether we should merge my stringbufs or > rusty's version? I have no particular preference. My vague recollection was that I preferred Rusty's. But it's been a while. -- 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
On Fri, Jan 18, 2008 at 12:41:08PM -0600, Matt Mackall wrote: > On Wed, 2008-01-16 at 10:00 +0900, Tejun Heo wrote: > > And mprintk the following. > > > > code: > > DEFINE_MPRINTK(mp, 2 * 80); > > > > mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > > mprintk_push(&mp, "ATA %d", 7); > > mprintk_push(&mp, ", %u sectors\n", 1024); > > mprintk(&mp, "everything seems dandy\n"); > > I prefer Matthew Wilcox's stringbuf approach which does proper memory > management and isn't specific to printk: > > http://www.ussg.iu.edu/hypermail/linux/kernel/0710.3/0517.html Do you have any strong feelings whether we should merge my stringbufs or rusty's version? I have no particular preference. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
On Wed, 2008-01-16 at 10:00 +0900, Tejun Heo wrote: > And mprintk the following. > > code: > DEFINE_MPRINTK(mp, 2 * 80); > > mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > mprintk_push(&mp, "ATA %d", 7); > mprintk_push(&mp, ", %u sectors\n", 1024); > mprintk(&mp, "everything seems dandy\n"); I prefer Matthew Wilcox's stringbuf approach which does proper memory management and isn't specific to printk: http://www.ussg.iu.edu/hypermail/linux/kernel/0710.3/0517.html -- 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
Hello, Randy Dunlap wrote: >> mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); >> mprintk_push(&mp, "ATA %d", 7); >> mprintk_push(&mp, ", %u sectors\n", 1024); >> mprintk(&mp, "everything seems dandy\n"); > > Looks pretty good to me except that I would change mprintk_push to > mprintk_add or mprintk_append (I think that I prefer _add). I think push and flush sound good when used together but then again the flush function isn't visible in the interface and push has LIFO ring to it. I'm okay with add. append seems a bit too long. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
On Wed, 16 Jan 2008 10:00:06 +0900 Tejun Heo wrote: > Hello, all. > > This patchset implements printk_header() and mprintk - merging printk > - to make printing multiline messages and assembling message > piece-by-piece easier. > > In a nutshell, printk_header() lets you do the following atomically > (against other messages). > > code: + printk_header(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); > > output: > <6>ata1.00: line0 > <6> line1 > <6> line2 > > And mprintk the following. > > code: > DEFINE_MPRINTK(mp, 2 * 80); > > mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); > mprintk_push(&mp, "ATA %d", 7); > mprintk_push(&mp, ", %u sectors\n", 1024); > mprintk(&mp, "everything seems dandy\n"); Looks pretty good to me except that I would change mprintk_push to mprintk_add or mprintk_append (I think that I prefer _add). > output: > <6>ata1.00: ATA 7, 1024 sectors > <6> everything seems dandy > > Please read the commit messages and comments for more detail. If this > patchset is accepted, I'll write up Documentation/printk.txt which > contains describtion of the API and guidelines - "don't pack unrelated > messages into one" kind of stuff. > > This patchset is against the current linux-2.6#master (031f2dcd) and > contains the following patches. > > 0001-printk-keep-log-level-on-multiline-messages.patch > 0002-printk-implement-v-printk_header.patch > 0003-printk-implement-merging-printk.patch > 0004-libata-make-libata-use-printk_header-and-mprintk.patch > > drivers/ata/libata-core.c | 202 +++-- > drivers/ata/libata-eh.c | 150 -- > drivers/ata/libata-pmp.c|5 > drivers/ata/libata-scsi.c |6 > drivers/ata/sata_inic162x.c |2 > drivers/ata/sata_nv.c |4 > include/linux/kernel.h | 83 ++ > include/linux/libata.h | 35 ++-- > kernel/printk.c | 354 > > 9 files changed, 630 insertions(+), 211 deletions(-) > > More than half of the code increase in kernel.h are from the dummy > declarations for !CONFIG_PRINTK. More than one third of printk.c > increase are comments. On my x86-64 configuration, printk.o grows > from 30152 to 34128. > > libata code grows slightly but the increase is from converting the > printk wrapper from #define to proper functions. The converted areas > - device configuration and EH reporting - were reduced in comlexity > and size. With all-y, drivers/ata/built-in.o shrinks from 726509 to > 717657 mostly due to the conversion away from macros. --- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHSET] printk: implement printk_header() and merging printk
Tejun Heo wrote: > Hello, all. > > This patchset implements printk_header() and mprintk - merging printk > - to make printing multiline messages and assembling message > piece-by-piece easier. > > In a nutshell, printk_header() lets you do the following atomically > (against other messages). > > code: > printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); That should have been printk_header instead of printk. Sorry. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHSET] printk: implement printk_header() and merging printk
Hello, all. This patchset implements printk_header() and mprintk - merging printk - to make printing multiline messages and assembling message piece-by-piece easier. In a nutshell, printk_header() lets you do the following atomically (against other messages). code: printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); output: <6>ata1.00: line0 <6> line1 <6> line2 And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); mprintk_push(&mp, "ATA %d", 7); mprintk_push(&mp, ", %u sectors\n", 1024); mprintk(&mp, "everything seems dandy\n"); output: <6>ata1.00: ATA 7, 1024 sectors <6> everything seems dandy Please read the commit messages and comments for more detail. If this patchset is accepted, I'll write up Documentation/printk.txt which contains describtion of the API and guidelines - "don't pack unrelated messages into one" kind of stuff. This patchset is against the current linux-2.6#master (031f2dcd) and contains the following patches. 0001-printk-keep-log-level-on-multiline-messages.patch 0002-printk-implement-v-printk_header.patch 0003-printk-implement-merging-printk.patch 0004-libata-make-libata-use-printk_header-and-mprintk.patch drivers/ata/libata-core.c | 202 +++-- drivers/ata/libata-eh.c | 150 -- drivers/ata/libata-pmp.c|5 drivers/ata/libata-scsi.c |6 drivers/ata/sata_inic162x.c |2 drivers/ata/sata_nv.c |4 include/linux/kernel.h | 83 ++ include/linux/libata.h | 35 ++-- kernel/printk.c | 354 9 files changed, 630 insertions(+), 211 deletions(-) More than half of the code increase in kernel.h are from the dummy declarations for !CONFIG_PRINTK. More than one third of printk.c increase are comments. On my x86-64 configuration, printk.o grows from 30152 to 34128. libata code grows slightly but the increase is from converting the printk wrapper from #define to proper functions. The converted areas - device configuration and EH reporting - were reduced in comlexity and size. With all-y, drivers/ata/built-in.o shrinks from 726509 to 717657 mostly due to the conversion away from macros. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
