Messages in this thread | | | From | Kay Sievers <> | Date | Wed, 11 Jul 2012 19:25:24 +0200 | Subject | Re: pr_cat() + CATSTR(name, size)? |
| |
On Wed, Jul 11, 2012 at 6:55 PM, Joe Perches <joe@perches.com> wrote: > On Wed, 2012-07-11 at 17:48 +0200, Kay Sievers wrote: >> On Wed, Jul 11, 2012 at 5:30 PM, Joe Perches <joe@perches.com> wrote: >> > Well, I think the malloc costs are pretty low >> > and could devolve pretty easily when OOM. >> >> We need to avoid allocating memory in situations where we want to >> printk(), it's just not possible. > > "it's just not possible???" Kay, them's fightin' words. :)
Nah, I meant it. :) It limits the usefulness of these functions. We can not safely allocate memory, or do not get any memory in some situations where we want to use printk(). Hey, it might be used to say printk("out of memory\n").
>> That's why all the kmsg/printk can >> not really do any plain malloc. All printk memory needs to be static, >> on the stack or somehow pre-allocated. > > Maybe, I was planning to play with it after > refactoring printk in the next couple releases.
Sounds good.
>> > Anyway, interesting idea, keep at it, see what >> > comes out of it. >> >> Just depends on us, I guess. :)
> If your solution is just for the dev_<level> messages > (ie: with vprintk_emit descriptors), then it's not > too ugly.
Yeah, I thought only about these. But there might be more users where it makes sense to do that in a more reliable manner, don't know. It was surely no meant to replace the remaining 99.9% of the other cont users. :)
> Did you look at the remaining dev_<level> and printk > continuations grep pattern? There really aren't too > many to fix up.
Yeah, it looks fine to fix these few.
> Maybe in 3.6. None of them appear particularly urgent.
Right.
> One trivial style note: > > Maybe CATSTR could use a struct and a DECLARE_ macro? > > struct printk_continuation_buffer { > size_t length; > size_t pos; > char buf[]; > }
Yeah, but then we lose the simplicity of passing the normal string around, and we need accessor macros to get to the string when we pass it around later. Maybe it's still OK, but it's surely not so intuitive anymore.
> It's a pity gcc doesn't allow non-static declarations like: > > #define DECLARE_PRINTK_BUF(name, size) \ > struct printk_continuation_buffer name = { \ > .length = size; \ > .pos = 0; \ > .buf[size] = {0}; \ > }
Yeah, when the size changes, we have different type of struct. So we can not name them all "printk_continuation_buffer", every different size would conflict with each other.
Also = {0} on an array forces a memset() of the entire array, nothing wrong, but it's not really needed.
> So maybe a DECLARE/DESTROY thing could work > with the appropriate malloc/free.
Hmm, I really don't think we can teach the people, or expect them to know, that these printk() functions are fragile if used in some critical code paths. It would at least need the GFP flags and in many cases GFP_ATOMIC which can easily fail, and we would also need to do error checking then, and printk() should just never fail, because it is used to tell that something went wrong. We have the entire kmsg buffer pre-allocated at bootup for that reason.
I think the only really sane option here is to use the (usually ~50-100 bytes) stack. Or did you have another idea here which I missed?
Kay
| |