Message-ID: <66cea3bf3332f_f937b29424@iweiny-mobl.notmuch>
Date: Tue, 27 Aug 2024 23:12:47 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Ira Weiny
	<ira.weiny@...el.com>
CC: Petr Mladek <pmladek@...e.com>, Dave Jiang <dave.jiang@...el.com>, Fan Ni
	<fan.ni@...sung.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	"Navneet Singh" <navneet.singh@...el.com>, Chris Mason <clm@...com>, Josef
 Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, Steven Rostedt
	<rostedt@...dmis.org>, Rasmus Villemoes <linux@...musvillemoes.dk>, "Sergey
 Senozhatsky" <senozhatsky@...omium.org>, Jonathan Corbet <corbet@....net>,
	Andrew Morton <akpm@...ux-foundation.org>, Dan Williams
	<dan.j.williams@...el.com>, Davidlohr Bueso <dave@...olabs.net>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, <linux-btrfs@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <nvdimm@...ts.linux.dev>
Subject: 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 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(-)


> 
> 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.

> 
> > +       *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.

Good.

> 
> > Most importantly that would not be much less code AFAICT.
> 
> ...
> 
> > +       %par    [range 0x0000000060000000-0x000000006fffffff]
> 
> I still think this is not okay to use %pa namespace.

Agreed.  Lets go with %pra

> 
> ...
> 
> > +static void __init
> > +struct_range(void)
> > +{
> > +       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 0x0000000000c0ffee-0x00000000ba5eba11]",
> > +            "%par", &test_range);
> 
> Case when start == end?

Yes, that is the 1st case.

> Case when end < start?

I had no intention of having the output dictated by the values.

	test("[range 0x0000000000c0ffee-0x0000000000c0ffee]",
and
	test("[range 0x00000000ba5eba11-0x0000000000c0ffee]",

... are acceptable to me.

> 
> > +}
> 
> ...
> 
> > +       *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';
> 
> As per above comments.


Thanks for the review,
Ira

[1] sample diff

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6be1ca13790c..84757e75e047 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1039,6 +1039,18 @@ static const struct printf_spec default_dec04_spec = {
        .flags = ZEROPAD,
 };
 
+static noinline_for_stack
+char *hex_range(char *buf, char *end, u64 start_val, u64 end_val,
+               struct printf_spec spec)
+{
+       buf = number(buf, end, start_val, spec);
+       if (start_val != end_val) {
+               *buf++ = '-';
+               buf = number(buf, end, end_val, spec);
+       }
+       return buf;
+}
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
                      struct printf_spec spec, const char *fmt)
@@ -1115,11 +1127,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
                p = string_nocheck(p, pend, "size ", str_spec);
                p = number(p, pend, resource_size(res), *specp);
        } else {
-               p = number(p, pend, res->start, *specp);
-               if (res->start != res->end) {
-                       *p++ = '-';
-                       p = number(p, pend, res->end, *specp);
-               }
+               p = hex_range(p, pend, res->start, res->end, *specp);
        }
        if (decode) {
                if (res->flags & IORESOURCE_MEM_64)
@@ -1149,11 +1157,19 @@ char *range_string(char *buf, char *end, const struct range *range,
        char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
        char *p = sym, *pend = sym + sizeof(sym);
 
+       struct printf_spec range_spec = {
+               spec.field_width = 2 + 2 * sizeof(range->start), /* 0x + 2 * u64 */
+               spec.flags = SPECIAL | SMALL | ZEROPAD,
+               spec.base = 16,
+               spec.precision = -1,
+       };
+
+       if (check_pointer(&buf, end, range, spec))
+               return buf;
+
        *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 = hex_range(p, pend, range->start, range->end, range_spec);
        *p++ = ']';
        *p = '\0';
 



[2] sample diff

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a754eefef252..e6870eb703a4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1149,11 +1149,16 @@ char *range_string(char *buf, char *end, const struct range *range,
        char sym[RANGE_DECODED_BUF_SIZE + RANGE_PRINT_BUF_SIZE];
        char *p = sym, *pend = sym + sizeof(sym);

+       if (check_pointer(&buf, end, range, spec))
+               return buf;
+
        *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));
+       if (range->start != range->end) {
+               *p++ = '-';
+               p = special_hex_number(p, pend, range->end, sizeof(range->end));
+       }
        *p++ = ']';
        *p = '\0';

Powered by blists - more mailing lists