Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Tue, Aug 27, 2024 at 11:12:47PM -0500, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote:
> > > Andy Shevchenko wrote:
> > > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > > > Petr Mladek wrote:
> > > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
[snip]
> > > +char *range_string(char *buf, char *end, const struct range *range,
> > > + struct printf_spec spec, const char *fmt)
> > > +{
> > > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> > > +#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
> > > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> > > + char *p = sym, *pend = sym + sizeof(sym);
> >
> > Missing check for pointer, but it's not that I wanted to tell.
>
> No it was not missing. It was checked in address_val() already. However,
> with
> %pra I'll have to add it in.
Ah, I haven't noticed the address_val() implementation details, thanks for
elaborating!
> > > + *p++ = '[';
> > > + p = string_nocheck(p, pend, "range ", default_str_spec);
> >
> > Hmm... %pr uses str_spec, what the difference can be here?
>
> str_spec is designed for variable length strings which are used based on the
> struct resource flags. Struct range does not vary so default_str_spec works.
Okay, makes sense.
> > > + p = special_hex_number(p, pend, range->start,
> > > sizeof(range->start));
> > > + *p++ = '-';
> > > + p = special_hex_number(p, pend, range->end, sizeof(range->end));
> >
> > This is basically the copy of %pr implementation.
>
> Only at a very basic level. struct resource has a variable spec while struct
> range does not. This causes complexity to make the code the same.
Fair enough, that's why I said "as much as possible to deduplicate". If you
think this is not worth it, let's do without an additional complications then.
> > p = number(p, pend, res->start, *specp);
> > if (res->start != res->end) {
> > *p++ = '-';
> > p = number(p, pend, res->end, *specp);
> > }
> >
> > Would it be possible to unify? I think so, but it requires a bit of
> > thinking.
>
> Not much thinking. But the issue is that they are not close enough to justify
> the extra complexity IMHO.
Okay!
> Making the outputs match with a common function takes 13 lines of code[1]
> including the declaration of a print specification which, as this thread
> already showed, is non-trivial to understand.
> __Also__ this is currently crashing on me and I can't figure out why.
>
> $ git diff --stat
> lib/vsprintf.c | 32
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> OTOH to force a unified output, only takes 2 lines of duplicated code.[2]
> This
> is a very minor expense of duplicate code which is much easier to follow.
>
> $ git diff --stat
> lib/vsprintf.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Yep, got it.
> > That's why testing is very important in this kind of generic code.
>
> Yep. But the struct resource test was stubbed out. I've added some basic
> ones. But there are many more variations of struct resource prints. I'm not
> sure I've not broken them.
Yeah, so make it then separated branches for %pr and %pra. You will take the
correct argument type in each of them. There are existing examples there.
Probably an initial 'r'/'R' parsing should be moved to pointer().
> > > + *p++ = ']';
> > > + *p = '\0';
> > > +
> > > + return string_nocheck(buf, end, sym, spec);
> > > +}
...
> > > + struct range test_range = {
> > > + .start = 0xc0ffee00ba5eba11,
> > > + .end = 0xc0ffee00ba5eba11,
> > > + };
> > > +
> > > + test("[range 0xc0ffee00ba5eba11-0xc0ffee00ba5eba11]",
> > > +"%par", &test_range);
> > > +
> > > + test_range = (struct range) {
> > > + .start = 0xc0ffee,
> > > + .end = 0xba5eba11,
> > > + };
> > > + test("[range 0x00c0ffee-0xba5eba11]",
> > > +"%par", &test_range);
> >
> > Case when start == end?
>
> Yes, that is the 1st case.
Thumb up!
> > Case when end < start?
>
> I had no intention of having the output dictated by the values.
>
> test("[range 0x00c0ffee-0x00c0ffee]",
> and
> test("[range 0xba5eba11-0x00c0ffee]",
>
> ... are acceptable to me.
But it seems the %pr in the first case doesn't do range, just a single value,
which makes sense to me (and this thread proved it) to avoid needless pedantic
checking of each value. It means that at a glance you may tell start == end.
Not sure about end < start case, but the point is just let's make it mimicing
%pr behaviour.
...
>
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
Andy Shevchenko wrote:
> On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote:
> > Andy Shevchenko wrote:
> > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > > Petr Mladek wrote:
> > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
>
[snip]
> > > > >
> > > > > With that said, I'm not sure the %pa is a good placeholder for this
> > > > > ('a' stands
> > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
> >
> > I'm speaking a bit for Dan here but also the logical way I thought of
> > things.
> >
> > 1) %p does not dictate anything about the format of the data. Rather
> >indicates that what is passed is a pointer. Because we are passing a
> >pointer to a range struct %pXX makes sense.
>
> There is no objection to that.
>
> > 2) %pa indicates what follows is 'address'. This was a bit of creative
> >license because, as I said in the commit message most of the time
> >struct range contains an address range. So for this narrow use case it
> >also makes sense.
>
> As in the discussion it was pointed out that struct range is always 64-bit,
> limiting it to the "address" is a wrong assumption as we are talking generic
> printing routine here. We don't know what users will be in the future on
> 32-bit
> platforms, or what data (semantically) is being held by this structure.
>
> > 3) %par r for range.
>
> I understand, but again struct range != address.
Agreed.
>
> > %p[rR] is taken.
> > %pra confuses things IMO.
>
> It doesn't confuse me. :-) But I believe Petr also has a rationale behind this
> proposal as he described earlier.
%pra it is then.
>
> > > > The r/R in %pr/%pR actually stands for "resource".
> > > >
> > > > But "%ra" really looks like a better choice than "%par". Both
> > > > "resource" and "range" starts with 'r'. Also the struct resource
> > > > is printed as a range of values.
> >
> > %r could be used I think. But this breaks with the convention of passing a
> > pointer and how to interpret it. The other idea I had, mentioned in the
> > commit
> > message was %pn. Meaning passed by pointer 'raNge'.
>
> No, we can't use %r or anything else that is documented for the standard
> printf() format specifiers, otherwise you will get a compiler warning and
> basically it means no go.
I was not thrilled with %r anyway.
>
> > I think that follows better than %r. That would be another break from C99.
> > But we don't have to follow that.
> >
> > > Fine with me as long as it:
> > > 1) doesn't collide with %pa namespace
> > > 2) tries to deduplicate existing code as much as possible.
> >
> > Andy, I'm not quite following how you expect to share the code between
> > resource_string() and range_string()?
> >
> > There is very little duplicated code. In fact with Petr's suggestions and
> > some
> > more work range_string() is quite simple:
> >
> > +static noinline_for_stack
> > +char *range_string(char *buf, char *end, const struct range *range,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> > +#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
> > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> > + char *p = sym, *pend = sym + sizeof(sym);
>
>
> Missing check for pointer, but it's not that I wanted to tell.
No it was not missing. It was checked in address_val() already. However, with
%pra I'll have to add it in.
>
> > + *p++ = '[';
> > + p = string_nocheck(p, pend, "range ", default_str_spec);
>
> Hmm... %pr uses str_spec, what the difference can be here?
str_spec is designed for variable length strings which are used based on the
struct resource flags. Struct range does not vary so default_str_spec works.
>
> > + p = special_hex_number(p, pend, range->start, sizeof(range->start));
> > + *p++ = '-';
> > + p = special_hex_number(p, pend, range->end, sizeof(range->end));
>
> This is basically the copy of %pr implementation.
Only at a very basic level. struct resource has a variable spec while struct
range does not. This causes complexity to make the code the same.
>
> p = number(p, pend, res->start, *specp);
> if (res->start != res->end) {
> *p++ = '-';
> p = number(p, pend, res->end, *specp);
> }
>
> Would it be possible to unify? I think so, but it requires a bit of thinking.
Not much thinking. But the issue is that they are not close enough to justify
the extra complexity IMHO.
Making the outputs match with a common function takes 13 lines of code[1]
including the declaration of a print specification which, as this thread
already showed, is non-trivial to understand.
__Also__ this is currently crashing on me and I can't figure out wh
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
Petr Mladek wrote:
> On Mon 2024-08-26 16:17:52, Ira Weiny wrote:
> > Andy Shevchenko wrote:
> > > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > > Petr Mladek wrote:
> > > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
> > >
> > > ...
> > >
> > > > > > > > + %par[range 0x6000-0x6fff] or
> > > > > > >
> > > > > > > It seems that it is always 64-bit. It prints:
> > > > > > >
> > > > > > > struct range {
> > > > > > > u64 start;
> > > > > > > u64 end;
> > > > > > > };
> > > > > >
> > > > > > Indeed. Thanks I should not have just copied/pasted.
> > > > >
> > > > > With that said, I'm not sure the %pa is a good placeholder for this
> > > > > ('a' stands
> > > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
> >
> > I'm speaking a bit for Dan here but also the logical way I thought of
> > things.
> >
> > 1) %p does not dictate anything about the format of the data. Rather
> >indicates that what is passed is a pointer. Because we are passing a
> >pointer to a range struct %pXX makes sense.
> > 2) %pa indicates what follows is 'address'. This was a bit of creative
> >license because, as I said in the commit message most of the time
> >struct range contains an address range. So for this narrow use case it
> >also makes sense.
> > 3) %par r for range.
>
> Yes. I got it.
>
> Well, is struct range really used for addresses?
Commonly yes. But I agree with Andy that it is not always.
> It rather looks like
> a range of any 64-bit values.
>
> > %p[rR] is taken. %pra confuses things IMO.
>
> Another variants might be %pr64 or %prange.
>
> IMHO, there is no good solution. We are trying to find the least
> bad one. The meaning should be as obvious and as least confusing
> as possible.
Yep.
>
> Honestly, I do not have a strong opinion. I kind of like %prange ;-)
> But I could live with all other variants, except for %pn mentioned below.
>
> > > > The r/R in %pr/%pR actually stands for "resource".
> > > >
> > > > But "%ra" really looks like a better choice than "%par". Both
> > > > "resource" and "range" starts with 'r'. Also the struct resource
> > > > is printed as a range of values.
> >
> > %r could be used I think. But this breaks with the convention of passing a
> > pointer and how to interpret it.
>
> How exactly does it break the convention, please?
>
> Do you passing a pointer to struct range instead of a pointer to
> struct resource?
Yes a pointer is passed as the parameter. This is what %p means AFAIU.
Then the modifier is applied to know what we are pointing to.
>
> It should not be a big problem as long as the vsprintf() code is
> able to guess the right pointer type from the %pXX modifier.
>
> > The other idea I had, mentioned in the commit
> > message was %pn. Meaning passed by pointer 'raNge'.
>
> This looks like the worst variant to me.
Fair enough.
>
> > > Fine with me as long as it:
> > > 1) doesn't collide with %pa namespace
> > > 2) tries to deduplicate existing code as much as possible.
> >
> > Andy, I'm not quite following how you expect to share the code between
> > resource_string() and range_string()?
> >
> > There is very little duplicated code. In fact with Petr's suggestions and
> > some
> > more work range_string() is quite simple:
> >
> > +static noinline_for_stack
> > +char *range_string(char *buf, char *end, const struct range *range,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> > +#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
> > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> > + char *p = sym, *pend = sym + sizeof(sym);
> > +
> > + *p++ = '[';
> > + p = string_nocheck(p, pend, "range ", default_str_spec);
> > + p = special_hex_number(p, pend, range->start, sizeof(range->start));
> > + *p++ = '-';
> > + p = special_hex_number(p, pend, range->end, sizeof(range->end));
> > + *p++ = ']';
> > + *p = '\0';
> > +
> > + return string_nocheck(buf, end, sym, spec);
> > +}
>
> I agree that there is not much duplicated code in the end.
>
> > Also this is the bulk of the patch except for documentation and the new
> > testing code. [new patch below]
> >
> > Am I missing your point somehow? I considered cramming a struct range into
> > a
> > struct resource to let resource_string() process the data. But that would
> > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow
> > for the larger u64 data in struct range should this be a 32 bit physical
> > address config.
>
> This would be nasty. I believe that this is not what Andy meant.
Nope.
>
> Best Regards,
> Petr
>
> PS: I have vacation until the end of the w
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Tue, Aug 27, 2024 at 09:43:32AM +0200, Petr Mladek wrote: > On Mon 2024-08-26 16:17:52, Ira Weiny wrote: > > Andy Shevchenko wrote: ... > But I could live with all other variants, except for %pn mentioned below. I believe %r is also no go as we most likely get a complier warning. ... > > Am I missing your point somehow? I considered cramming a struct range into > > a > > struct resource to let resource_string() process the data. But that would > > involve creating a new IORESOURCE_* flag (not ideal) and also does not allow > > for the larger u64 data in struct range should this be a 32 bit physical > > address config. > > This would be nasty. I believe that this is not what Andy meant. You are right, this is not what I meant. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Mon, Aug 26, 2024 at 04:17:52PM -0500, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > Petr Mladek wrote:
> > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
...
> > > > > > > + %par[range 0x6000-0x6fff] or
> > > > > >
> > > > > > It seems that it is always 64-bit. It prints:
> > > > > >
> > > > > > struct range {
> > > > > > u64 start;
> > > > > > u64 end;
> > > > > > };
> > > > >
> > > > > Indeed. Thanks I should not have just copied/pasted.
> > > >
> > > > With that said, I'm not sure the %pa is a good placeholder for this
> > > > ('a' stands
> > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
>
> I'm speaking a bit for Dan here but also the logical way I thought of
> things.
>
> 1) %p does not dictate anything about the format of the data. Rather
>indicates that what is passed is a pointer. Because we are passing a
>pointer to a range struct %pXX makes sense.
There is no objection to that.
> 2) %pa indicates what follows is 'address'. This was a bit of creative
>license because, as I said in the commit message most of the time
>struct range contains an address range. So for this narrow use case it
>also makes sense.
As in the discussion it was pointed out that struct range is always 64-bit,
limiting it to the "address" is a wrong assumption as we are talking generic
printing routine here. We don't know what users will be in the future on 32-bit
platforms, or what data (semantically) is being held by this structure.
> 3) %par r for range.
I understand, but again struct range != address.
> %p[rR] is taken.
> %pra confuses things IMO.
It doesn't confuse me. :-) But I believe Petr also has a rationale behind this
proposal as he described earlier.
> > > The r/R in %pr/%pR actually stands for "resource".
> > >
> > > But "%ra" really looks like a better choice than "%par". Both
> > > "resource" and "range" starts with 'r'. Also the struct resource
> > > is printed as a range of values.
>
> %r could be used I think. But this breaks with the convention of passing a
> pointer and how to interpret it. The other idea I had, mentioned in the
> commit
> message was %pn. Meaning passed by pointer 'raNge'.
No, we can't use %r or anything else that is documented for the standard
printf() format specifiers, otherwise you will get a compiler warning and
basically it means no go.
> I think that follows better than %r. That would be another break from C99.
> But we don't have to follow that.
>
> > Fine with me as long as it:
> > 1) doesn't collide with %pa namespace
> > 2) tries to deduplicate existing code as much as possible.
>
> Andy, I'm not quite following how you expect to share the code between
> resource_string() and range_string()?
>
> There is very little duplicated code. In fact with Petr's suggestions and
> some
> more work range_string() is quite simple:
>
> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> + struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
> + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> + char *p = sym, *pend = sym + sizeof(sym);
Missing check for pointer, but it's not that I wanted to tell.
> + *p++ = '[';
> + p = string_nocheck(p, pend, "range ", default_str_spec);
Hmm... %pr uses str_spec, what the difference can be here?
> + p = special_hex_number(p, pend, range->start, sizeof(range->start));
> + *p++ = '-';
> + p = special_hex_number(p, pend, range->end, sizeof(range->end));
This is basically the copy of %pr implementation.
p = number(p, pend, res->start, *specp);
if (res->start != res->end) {
*p++ = '-';
p = number(p, pend, res->end, *specp);
}
Would it be possible to unify? I think so, but it requires a bit of thinking.
That's why testing is very important in this kind of generic code.
> + *p++ = ']';
> + *p = '\0';
> +
> + return string_nocheck(buf, end, sym, spec);
> +}
>
> Also this is the bulk of the patch except for documentation and the new
> testing code. [new patch below]
>
> Am I missing your point somehow?
See above.
> I considered cramming a struct range into a
> struct resource to let resource_string() process the data. But that would
> involve creating a new IORESOURCE_* flag (not ideal) and also does not allow
> for the larger u64 data in struct range should this be a 32 bit physical
> address config.
No, that's not what I was expecting.
> Most importantly that would not be much less code AFAICT.
...
> + %par[r
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Mon 2024-08-26 16:17:52, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > > Petr Mladek wrote:
> > > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
> >
> > ...
> >
> > > > > > > + %par[range 0x6000-0x6fff] or
> > > > > >
> > > > > > It seems that it is always 64-bit. It prints:
> > > > > >
> > > > > > struct range {
> > > > > > u64 start;
> > > > > > u64 end;
> > > > > > };
> > > > >
> > > > > Indeed. Thanks I should not have just copied/pasted.
> > > >
> > > > With that said, I'm not sure the %pa is a good placeholder for this
> > > > ('a' stands
> > > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
>
> I'm speaking a bit for Dan here but also the logical way I thought of
> things.
>
> 1) %p does not dictate anything about the format of the data. Rather
>indicates that what is passed is a pointer. Because we are passing a
>pointer to a range struct %pXX makes sense.
> 2) %pa indicates what follows is 'address'. This was a bit of creative
>license because, as I said in the commit message most of the time
>struct range contains an address range. So for this narrow use case it
>also makes sense.
> 3) %par r for range.
Yes. I got it.
Well, is struct range really used for addresses? It rather looks like
a range of any 64-bit values.
> %p[rR] is taken. %pra confuses things IMO.
Another variants might be %pr64 or %prange.
IMHO, there is no good solution. We are trying to find the least
bad one. The meaning should be as obvious and as least confusing
as possible.
Honestly, I do not have a strong opinion. I kind of like %prange ;-)
But I could live with all other variants, except for %pn mentioned below.
> > > The r/R in %pr/%pR actually stands for "resource".
> > >
> > > But "%ra" really looks like a better choice than "%par". Both
> > > "resource" and "range" starts with 'r'. Also the struct resource
> > > is printed as a range of values.
>
> %r could be used I think. But this breaks with the convention of passing a
> pointer and how to interpret it.
How exactly does it break the convention, please?
Do you passing a pointer to struct range instead of a pointer to
struct resource?
It should not be a big problem as long as the vsprintf() code is
able to guess the right pointer type from the %pXX modifier.
> The other idea I had, mentioned in the commit
> message was %pn. Meaning passed by pointer 'raNge'.
This looks like the worst variant to me.
> > Fine with me as long as it:
> > 1) doesn't collide with %pa namespace
> > 2) tries to deduplicate existing code as much as possible.
>
> Andy, I'm not quite following how you expect to share the code between
> resource_string() and range_string()?
>
> There is very little duplicated code. In fact with Petr's suggestions and
> some
> more work range_string() is quite simple:
>
> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> + struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
> + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> + char *p = sym, *pend = sym + sizeof(sym);
> +
> + *p++ = '[';
> + p = string_nocheck(p, pend, "range ", default_str_spec);
> + p = special_hex_number(p, pend, range->start, sizeof(range->start));
> + *p++ = '-';
> + p = special_hex_number(p, pend, range->end, sizeof(range->end));
> + *p++ = ']';
> + *p = '\0';
> +
> + return string_nocheck(buf, end, sym, spec);
> +}
I agree that there is not much duplicated code in the end.
> Also this is the bulk of the patch except for documentation and the new
> testing code. [new patch below]
>
> Am I missing your point somehow? I considered cramming a struct range into a
> struct resource to let resource_string() process the data. But that would
> involve creating a new IORESOURCE_* flag (not ideal) and also does not allow
> for the larger u64 data in struct range should this be a 32 bit physical
> address config.
This would be nasty. I believe that this is not what Andy meant.
Best Regards,
Petr
PS: I have vacation until the end of the week, so my next eventual
reaction would be delayed.
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
Andy Shevchenko wrote:
> On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> > On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > > Petr Mladek wrote:
> > > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
>
> ...
>
> > > > > > + %par[range 0x6000-0x6fff] or
> > > > >
> > > > > It seems that it is always 64-bit. It prints:
> > > > >
> > > > > struct range {
> > > > > u64 start;
> > > > > u64 end;
> > > > > };
> > > >
> > > > Indeed. Thanks I should not have just copied/pasted.
> > >
> > > With that said, I'm not sure the %pa is a good placeholder for this ('a'
> > > stands
> > > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
I'm speaking a bit for Dan here but also the logical way I thought of
things.
1) %p does not dictate anything about the format of the data. Rather
indicates that what is passed is a pointer. Because we are passing a
pointer to a range struct %pXX makes sense.
2) %pa indicates what follows is 'address'. This was a bit of creative
license because, as I said in the commit message most of the time
struct range contains an address range. So for this narrow use case it
also makes sense.
3) %par r for range.
%p[rR] is taken. %pra confuses things IMO.
> >
> > The r/R in %pr/%pR actually stands for "resource".
> >
> > But "%ra" really looks like a better choice than "%par". Both
> > "resource" and "range" starts with 'r'. Also the struct resource
> > is printed as a range of values.
%r could be used I think. But this breaks with the convention of passing a
pointer and how to interpret it. The other idea I had, mentioned in the commit
message was %pn. Meaning passed by pointer 'raNge'.
I think that follows better than %r. That would be another break from C99.
But we don't have to follow that.
>
> Fine with me as long as it:
> 1) doesn't collide with %pa namespace
> 2) tries to deduplicate existing code as much as possible.
Andy, I'm not quite following how you expect to share the code between
resource_string() and range_string()?
There is very little duplicated code. In fact with Petr's suggestions and some
more work range_string() is quite simple:
+static noinline_for_stack
+char *range_string(char *buf, char *end, const struct range *range,
+ struct printf_spec spec, const char *fmt)
+{
+#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
+#define RANGE_PRINT_BUF_SIZE sizeof("[range -]")
+ char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
+ char *p = sym, *pend = sym + sizeof(sym);
+
+ *p++ = '[';
+ p = string_nocheck(p, pend, "range ", default_str_spec);
+ p = special_hex_number(p, pend, range->start, sizeof(range->start));
+ *p++ = '-';
+ p = special_hex_number(p, pend, range->end, sizeof(range->end));
+ *p++ = ']';
+ *p = '\0';
+
+ return string_nocheck(buf, end, sym, spec);
+}
Also this is the bulk of the patch except for documentation and the new
testing code. [new patch below]
Am I missing your point somehow? I considered cramming a struct range into a
struct resource to let resource_string() process the data. But that would
involve creating a new IORESOURCE_* flag (not ideal) and also does not allow
for the larger u64 data in struct range should this be a 32 bit physical
address config.
Most importantly that would not be much less code AFAICT.
Ira
[snip]
commit a5f0305d319eac7c6e480851378695f8bd42a3d0
Author: Ira Weiny
Date: Fri Jun 28 16:47:06 2024 -0500
printk: Add print format (%par) for struct range
The use of struct range in the CXL subsystem is growing. In particular,
the addition of Dynamic Capacity devices uses struct range in a number
of places which are reported in debug and error messages.
To wit requiring the printing of the start/end fields in each print
became cumbersome. Dan Williams mentions in [1] that it might be time
to have a print specifier for struct range similar to struct resource
A few alternatives were considered including '%pn' for 'print raNge' but
%par follows that struct range is most often used to store a range of
physical addresses. So use '%par' for 'print address range'.
To: Petr Mladek (maintainer:VSPRINTF)
To: Steven Rostedt (maintainer:VSPRINTF)
To: Jonathan Corbet (maintainer:DOCUMENTATION)
Cc: [email protected] (open list:DOCUMENTATION)
Cc: [email protected] (open list)
Link:
https://lore.kernel.org/all/[email protected]/
[1]
Suggested-by: "Dan Williams"
Signed-off-by: Ira Weiny
---
Changes:
[iweiny: use special_hex_number()]
[Petr: Update documentation]
[Petr: use 'range -']
[Petr: fixup printf_spec specifiers]
[Petr: add lib/test_printf test]
diff --git a/Documentat
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Mon, Aug 26, 2024 at 03:23:50PM +0200, Petr Mladek wrote:
> On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> > On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > > Petr Mladek wrote:
> > > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
...
> > > > > + %par[range 0x6000-0x6fff] or
> > > >
> > > > It seems that it is always 64-bit. It prints:
> > > >
> > > > struct range {
> > > > u64 start;
> > > > u64 end;
> > > > };
> > >
> > > Indeed. Thanks I should not have just copied/pasted.
> >
> > With that said, I'm not sure the %pa is a good placeholder for this ('a'
> > stands
> > to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
>
> The r/R in %pr/%pR actually stands for "resource".
>
> But "%ra" really looks like a better choice than "%par". Both
> "resource" and "range" starts with 'r'. Also the struct resource
> is printed as a range of values.
Fine with me as long as it:
1) doesn't collide with %pa namespace
2) tries to deduplicate existing code as much as possible.
> > > > > + [range 0x6000-0x6fff]
> > > > > +
> > > > > +For printing struct range. A variation of printing a physical
> > > > > address is to
> > > > > +print the value of struct range which are often used to hold a
> > > > > physical address
> > > > > +range.
> > > > > +
> > > > > +Passed by reference.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Mon, Aug 26, 2024 at 03:17:26PM +0200, Petr Mladek wrote:
> On Thu 2024-08-22 12:53:32, Ira Weiny wrote:
> > Petr Mladek wrote:
> > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
[...]
> > > > + static const struct printf_spec range_spec = {
> > > > + .base = 16,
> > > > + .field_width = RANGE_PRINTK_SIZE,
> >
> > However, my testing indicates this needs to be.
> >
> > .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */
>
> Makes sense. Great catch!
Which effectively means usage of special_hex_number().
But again, consider to unite this with %pR/r implementation(s).
> > ... to properly zero pad the value. Does that make sense?
> >
> > > > + .precision = -1,
> > > > + .flags = SPECIAL | SMALL | ZEROPAD,
> > > > + };
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Thu 2024-08-22 21:10:25, Andy Shevchenko wrote:
> On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> > Petr Mladek wrote:
> > > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
>
> ...
>
> > > > + %par[range 0x6000-0x6fff] or
> > >
> > > It seems that it is always 64-bit. It prints:
> > >
> > > struct range {
> > > u64 start;
> > > u64 end;
> > > };
> >
> > Indeed. Thanks I should not have just copied/pasted.
>
> With that said, I'm not sure the %pa is a good placeholder for this ('a'
> stands
> to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
The r/R in %pr/%pR actually stands for "resource".
But "%ra" really looks like a better choice than "%par". Both
"resource" and "range" starts with 'r'. Also the struct resource
is printed as a range of values.
> > > > + [range 0x6000-0x6fff]
> > > > +
> > > > +For printing struct range. A variation of printing a physical address
> > > > is to
> > > > +print the value of struct range which are often used to hold a
> > > > physical address
> > > > +range.
> > > > +
> > > > +Passed by reference.
Best Regards,
Petr
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Thu 2024-08-22 12:53:32, Ira Weiny wrote:
> Petr Mladek wrote:
> > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
> > > The use of struct range in the CXL subsystem is growing. In particular,
> > > the addition of Dynamic Capacity devices uses struct range in a number
> > > of places which are reported in debug and error messages.
> > >
> > > To wit requiring the printing of the start/end fields in each print
> > > became cumbersome. Dan Williams mentions in [1] that it might be time
> > > to have a print specifier for struct range similar to struct resource
> > >
> > > A few alternatives were considered including '%pn' for 'print raNge' but
> > > %par follows that struct range is most often used to store a range of
> > > physical addresses. So use '%par' for 'print address range'.
> > >
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 2d71b1115916..c132178fac07 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1140,6 +1140,39 @@ char *resource_string(char *buf, char *end, struct
> > > resource *res,
> > > return string_nocheck(buf, end, sym, spec);
> > > }
> > >
> > > +static noinline_for_stack
> > > +char *range_string(char *buf, char *end, const struct range *range,
> > > + struct printf_spec spec, const char *fmt)
> > > +{
> > > +#define RANGE_PRINTK_SIZE16
> > > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> > > +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]")
[...]
> > > + static const struct printf_spec range_spec = {
> > > + .base = 16,
> > > + .field_width = RANGE_PRINTK_SIZE,
>
> However, my testing indicates this needs to be.
>
> .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */
Makes sense. Great catch!
> ... to properly zero pad the value. Does that make sense?
>
> > > + .precision = -1,
> > > + .flags = SPECIAL | SMALL | ZEROPAD,
> > > + };
> > > +
> > > + *p++ = '[';
> > > + p = string_nocheck(p, pend, "range ", str_spec);
> > > + p = number(p, pend, range->start, range_spec);
> > > + *p++ = '-';
> > > + p = number(p, pend, range->end, range_spec);
> > > + *p++ = ']';
> > > + *p = '\0';
Best Regards,
Petr
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Thu, Aug 22, 2024 at 12:53:32PM -0500, Ira Weiny wrote:
> Petr Mladek wrote:
> > On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
...
> > > + %par[range 0x6000-0x6fff] or
> >
> > It seems that it is always 64-bit. It prints:
> >
> > struct range {
> > u64 start;
> > u64 end;
> > };
>
> Indeed. Thanks I should not have just copied/pasted.
With that said, I'm not sure the %pa is a good placeholder for this ('a' stands
to "address" AFAIU). Perhaps this should go somewhere under %pr/%pR?
> > > + [range 0x6000-0x6fff]
> > > +
> > > +For printing struct range. A variation of printing a physical address
> > > is to
> > > +print the value of struct range which are often used to hold a physical
> > > address
> > > +range.
> > > +
> > > +Passed by reference.
...
> > Is this really needed? What about using "default_str_spec" instead?
>
> Because I got confused and was coping from resource_string().
>
> Deleted now...
>
> > > + .field_width = RANGE_PRINTK_SIZE,
>
> However, my testing indicates this needs to be.
>
> .field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */
>
> ... to properly zero pad the value. Does that make sense?
Looking at this, moving under %pr/R should deduplicate the code, no?
I.o.w. better to use existing code for them to print struct range, no?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
Petr Mladek wrote:
> On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
> > The use of struct range in the CXL subsystem is growing. In particular,
> > the addition of Dynamic Capacity devices uses struct range in a number
> > of places which are reported in debug and error messages.
> >
> > To wit requiring the printing of the start/end fields in each print
> > became cumbersome. Dan Williams mentions in [1] that it might be time
> > to have a print specifier for struct range similar to struct resource
> >
> > A few alternatives were considered including '%pn' for 'print raNge' but
> > %par follows that struct range is most often used to store a range of
> > physical addresses. So use '%par' for 'print address range'.
> >
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -231,6 +231,20 @@ width of the CPU data path.
> >
> > Passed by reference.
> >
> > +Struct Range
> > +
> > +
> > +::
> > +
> > + %par[range 0x6000-0x6fff] or
>
> It seems that it is always 64-bit. It prints:
>
> struct range {
> u64 start;
> u64 end;
> };
Indeed. Thanks I should not have just copied/pasted.
>
> > + [range 0x6000-0x6fff]
> > +
> > +For printing struct range. A variation of printing a physical address is
> > to
> > +print the value of struct range which are often used to hold a physical
> > address
> > +range.
> > +
> > +Passed by reference.
> > +
> > DMA address types dma_addr_t
> >
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 2d71b1115916..c132178fac07 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1140,6 +1140,39 @@ char *resource_string(char *buf, char *end, struct
> > resource *res,
> > return string_nocheck(buf, end, sym, spec);
> > }
> >
> > +static noinline_for_stack
> > +char *range_string(char *buf, char *end, const struct range *range,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > +#define RANGE_PRINTK_SIZE 16
> > +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> > +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]")
>
> I think that it should be "[range -]"
Sounds good.
>
> > + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> > + char *p = sym, *pend = sym + sizeof(sym);
> > +
> > + static const struct printf_spec str_spec = {
> > + .field_width = -1,
> > + .precision = 10,
> > + .flags = LEFT,
> > + };
>
> Is this really needed? What about using "default_str_spec" instead?
Because I got confused and was coping from resource_string().
Deleted now...
>
> > + static const struct printf_spec range_spec = {
> > + .base = 16,
> > + .field_width = RANGE_PRINTK_SIZE,
However, my testing indicates this needs to be.
.field_width = 18, /* 2 (0x) + 2 * 8 (bytes) */
... to properly zero pad the value. Does that make sense?
> > + .precision = -1,
> > + .flags = SPECIAL | SMALL | ZEROPAD,
> > + };
> > +
> > + *p++ = '[';
> > + p = string_nocheck(p, pend, "range ", str_spec);
> > + p = number(p, pend, range->start, range_spec);
> > + *p++ = '-';
> > + p = number(p, pend, range->end, range_spec);
> > + *p++ = ']';
> > + *p = '\0';
> > +
> > + return string_nocheck(buf, end, sym, spec);
> > +}
> > +
> > static noinline_for_stack
> > char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> > const char *fmt)
>
> Also add a selftest into lib/test_printf.c, please.
Yes of course... Makes testing easier too.
Thanks,
Ira
>
> Best Regards,
> Petr
Re: [PATCH v3 02/25] printk: Add print format (%par) for struct range
On Fri 2024-08-16 09:44:10, Ira Weiny wrote:
> The use of struct range in the CXL subsystem is growing. In particular,
> the addition of Dynamic Capacity devices uses struct range in a number
> of places which are reported in debug and error messages.
>
> To wit requiring the printing of the start/end fields in each print
> became cumbersome. Dan Williams mentions in [1] that it might be time
> to have a print specifier for struct range similar to struct resource
>
> A few alternatives were considered including '%pn' for 'print raNge' but
> %par follows that struct range is most often used to store a range of
> physical addresses. So use '%par' for 'print address range'.
>
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -231,6 +231,20 @@ width of the CPU data path.
>
> Passed by reference.
>
> +Struct Range
> +
> +
> +::
> +
> + %par[range 0x6000-0x6fff] or
It seems that it is always 64-bit. It prints:
struct range {
u64 start;
u64 end;
};
> + [range 0x6000-0x6fff]
> +
> +For printing struct range. A variation of printing a physical address is to
> +print the value of struct range which are often used to hold a physical
> address
> +range.
> +
> +Passed by reference.
> +
> DMA address types dma_addr_t
>
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 2d71b1115916..c132178fac07 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1140,6 +1140,39 @@ char *resource_string(char *buf, char *end, struct
> resource *res,
> return string_nocheck(buf, end, sym, spec);
> }
>
> +static noinline_for_stack
> +char *range_string(char *buf, char *end, const struct range *range,
> + struct printf_spec spec, const char *fmt)
> +{
> +#define RANGE_PRINTK_SIZE16
> +#define RANGE_DECODED_BUF_SIZE ((2 * sizeof(struct range)) + 4)
> +#define RANGE_PRINT_BUF_SIZE sizeof("[range - ]")
I think that it should be "[range -]"
> + char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
> + char *p = sym, *pend = sym + sizeof(sym);
> +
> + static const struct printf_spec str_spec = {
> + .field_width = -1,
> + .precision = 10,
> + .flags = LEFT,
> + };
Is this really needed? What about using "default_str_spec" instead?
> + static const struct printf_spec range_spec = {
> + .base = 16,
> + .field_width = RANGE_PRINTK_SIZE,
> + .precision = -1,
> + .flags = SPECIAL | SMALL | ZEROPAD,
> + };
> +
> + *p++ = '[';
> + p = string_nocheck(p, pend, "range ", str_spec);
> + p = number(p, pend, range->start, range_spec);
> + *p++ = '-';
> + p = number(p, pend, range->end, range_spec);
> + *p++ = ']';
> + *p = '\0';
> +
> + return string_nocheck(buf, end, sym, spec);
> +}
> +
> static noinline_for_stack
> char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>const char *fmt)
Also add a selftest into lib/test_printf.c, please.
Best Regards,
Petr
