LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg Kroah-Hartman <firstname.lastname@example.org>
To: Yury Norov <email@example.com>
Cc: Andy Shevchenko <firstname.lastname@example.org>,
Andy Shevchenko <email@example.com>,
Barry Song <firstname.lastname@example.org>,
Andrew Morton <email@example.com>,
Linux Kernel Mailing List <firstname.lastname@example.org>,
Dave Hansen <email@example.com>,
Rasmus Villemoes <firstname.lastname@example.org>,
"Rafael J. Wysocki" <email@example.com>,
Randy Dunlap <firstname.lastname@example.org>,
Alexander Gordeev <email@example.com>,
Stefano Brivio <firstname.lastname@example.org>,
"Ma, Jianpeng" <email@example.com>,
Valentin Schneider <firstname.lastname@example.org>,
"Peter Zijlstra (Intel)" <email@example.com>,
Daniel Bristot de Oliveira <firstname.lastname@example.org>,
Guodong Xu <email@example.com>,
"Zengtao (B)" <firstname.lastname@example.org>,
email@example.com, Linuxarm <firstname.lastname@example.org>
Subject: Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases
Date: Thu, 22 Jul 2021 19:47:28 +0200 [thread overview]
Message-ID: <YPmvMJHouZFDHVds@kroah.com> (raw)
On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
> On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <email@example.com> wrote:
> > > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > > > result is greater than and less than 4KB.
> > > > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > > > non-zero which will happen when printed buf is larger than one page
> > > > > > > in sysfs bin_attribute.
> > > > > >
> > > > > > More test cases is always a good thing, thanks!
> > > > >
> > > > > Generally yes. But in this case... I believe, Barry didn't write that
> > > > > huge line below by himself. Most probably he copy-pasted the output of
> > > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > > and just enforces current behavior of snprintf.
> > > >
> > > > I'm not sure I got what you are telling me. The big line is to test
> > > > strings that are bigger than 4k.
> > >
> > > I'm trying to say that human are not able to verify correctness of
> > > this line. The test is supposed to check bitmap_print_to_buf(), but
> > > reference output itself is generated by bitmap_print_to_buf(). This
> > > test will always pass by design, even if there's an error somewhere
> > > in the middle, isn't it?
> > Then please manually check it to verify it is correct or not. Once we
> > have it verified, that's fine, it will remain static in this test for
> > always going forward.
> > That's what "oracles" are for, there is nothing wrong with this test
> > case or "proof" that I can see.
> > > >
> > > > ...
> > > >
> > > > > > > +static const char large_list __initconst = /* more than 4KB */
> > > > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > > > >
> > > > > I don't like this behavior of the code: each individual line is not a
> > > > > valid bitmap_list. I would prefer to split original bitmap and print
> > > > > list representation of parts in a compatible format; considering a
> > > > > receiving part of this splitting machinery.
> > > >
> > > > I agree that split is not the best here, but after all it's only 1
> > > > line and this is on purpose.
> > >
> > > What I see is that bitmap_print_to_buf() is called many times,
> > That is not what the above list shows at all, it's one long string all
> > together, only split up to make it easier for us to work with.
> > > and
> > > each time it returns something that is not a valid bitmap list string.
> > > If the caller was be able to concatenate all the lines returned by
> > > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > > case, why don't he use scnprintf("pbl") directly?
> > I do not understand the objection here at all. This series is fixing a
> > real problem that eeople are having
> I explicitly asked about an example of this problem. Barry answered in
> a great length, but the key points are:
> > > So, the root problem here is that some machines have so many CPUs that
> > > their cpumask text representation may not fit into the full page in the
> > > worst case. Is my understanding correct? Can you share an example of
> > > such configuration?
> > in my understanding, I have not found any machine which has really
> > caused the problem till now.
> > [...]
> > This doesn't really happen nowadays as the maximum
> > NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
> > is still smaller than 4KB page size.
> If it's not true, can you or Barry please share such an example?
So for a 4k page size, if you have every-other-cpu-enabled on x86, it
will overflow this, right?
And I have heard of systems much bigger than this as well. Why do you
not think that large number of CPUs are not around?
> > and your complaining about test
> > strings is _VERY_ odd.
> The test itself is bad, but it's a minor issue.
> My main complain is that the bitmap part of this series introduces a
> function that requires O(N^2) of CPU time and O(N) of memory to just
> print a string. The existing snprintf does this in O(N) and O(1)
> respectively. Additionally to that, the proposed function has some
> flaws in design.
Can you propose a better solution?
And is O(N^2) even an issue for this? Have you run it to determine the
cpu load for such a tiny thing? Why optimize something that no one has
even tried yet?
> > If you have an alternate solution, please propose it, otherwise I will
> > be taking this series in the next few days.
> I think I suggested a better solution in the thread for v4:
> > kasprintf() does everything you need. Why don't you use it directly in
> > your driver?
> I'm not that familiar to sysfs internals to submit a patch, but the
> idea in more details is like this:
> if (bitmap_string == NULL)
> bitmap_string = kasprintf(bitmap, ...);
> Where bitmap_string pointer may be stored in struct file, struct kobject,
> struct bit_attribute or where it's convenient.
No, we are not storing strings in a kobject, sorry.
next prev parent reply other threads:[~2021-07-22 17:47 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-15 11:58 [PATCH v7 0/4] use bin_attribute to break the size limitation of cpumap ABI Barry Song
2021-07-15 11:58 ` [PATCH v7 1/4] cpumask: introduce cpumap_print_to_buf to support large bitmask and list Barry Song
2021-07-15 15:28 ` Yury Norov
2021-07-15 21:08 ` Song Bao Hua (Barry Song)
2021-07-16 0:57 ` Song Bao Hua (Barry Song)
2021-07-15 11:58 ` [PATCH v7 2/4] topology: use bin_attribute to break the size limitation of cpumap ABI Barry Song
2021-07-16 8:49 ` Song Bao Hua (Barry Song)
2021-07-16 20:04 ` Yury Norov
2021-07-17 0:16 ` Song Bao Hua (Barry Song)
2021-07-17 1:12 ` Yury Norov
2021-07-19 9:07 ` andriy.shevchenko
2021-07-19 11:10 ` Song Bao Hua (Barry Song)
2021-07-19 17:10 ` Yury Norov
2021-07-21 9:30 ` Song Bao Hua (Barry Song)
2021-07-15 11:58 ` [PATCH v7 3/4] drivers/base/node.c: " Barry Song
2021-07-15 11:58 ` [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases Barry Song
2021-07-15 12:09 ` Andy Shevchenko
2021-07-15 20:47 ` Yury Norov
2021-07-15 21:32 ` Andy Shevchenko
2021-07-15 23:23 ` Yury Norov
2021-07-16 0:41 ` Song Bao Hua (Barry Song)
2021-07-21 11:40 ` Greg Kroah-Hartman
2021-07-22 17:09 ` Yury Norov
2021-07-22 17:47 ` Greg Kroah-Hartman [this message]
2021-07-22 18:27 ` Yury Norov
2021-07-22 18:36 ` Andy Shevchenko
2021-07-22 18:45 ` Greg Kroah-Hartman
2021-07-28 9:08 ` Song Bao Hua (Barry Song)
2021-07-28 9:24 ` Andy Shevchenko
2021-07-15 15:36 ` Yury Norov
2021-07-28 13:41 ` [PATCH v7 0/4] use bin_attribute to break the size limitation of cpumap ABI Greg KH
2021-07-28 14:53 ` Yury Norov
2021-07-28 15:25 ` Greg KH
2021-07-28 18:58 ` [PATCH] bitmap: extend comment to bitmap_print_to_buf Yury Norov
2021-07-29 6:04 ` Song Bao Hua (Barry Song)
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).