From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1399FC63797 for ; Thu, 22 Jul 2021 17:47:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E2E1061407 for ; Thu, 22 Jul 2021 17:47:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229837AbhGVRG4 (ORCPT ); Thu, 22 Jul 2021 13:06:56 -0400 Received: from mail.kernel.org ([198.145.29.99]:57626 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229536AbhGVRGz (ORCPT ); Thu, 22 Jul 2021 13:06:55 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id E8A9B6135A; Thu, 22 Jul 2021 17:47:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1626976050; bh=XdqCZirstICjdLHQfMitOvR6dwyJLMO9kLydMKAiTxw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dPrHhcDaxL+k0xd8mwaVXIDJKy5lOyIXTIhmyQ3ee4E4+mUNzgM7W4FQFky7lXyLk 5m0LfXM5SMpF+E7naINdYUw+OptCA5t/R6OcFwCHUBYeOMqrfIQulDRsQCsbhMD9rV hQkND0ZUR3xMnI06QMYIVmxiE5epkitAogEnmRMk= Date: Thu, 22 Jul 2021 19:47:28 +0200 From: Greg Kroah-Hartman To: Yury Norov Cc: Andy Shevchenko , Andy Shevchenko , Barry Song , Andrew Morton , Linux Kernel Mailing List , Dave Hansen , Rasmus Villemoes , "Rafael J. Wysocki" , Randy Dunlap , Alexander Gordeev , Stefano Brivio , "Ma, Jianpeng" , Valentin Schneider , "Peter Zijlstra (Intel)" , Daniel Bristot de Oliveira , Guodong Xu , tangchengchang@huawei.com, "Zengtao (B)" , yangyicong , tim.c.chen@linux.intel.com, Linuxarm Subject: Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases Message-ID: References: <20210715115856.11304-1-song.bao.hua@hisilicon.com> <20210715115856.11304-5-song.bao.hua@hisilicon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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: > > https://lore.kernel.org/lkml/4ab928f1fb3e4420974dfafe4b32f5b7@hisilicon.com/ > > > > 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: > > https://lore.kernel.org/lkml/YMu2amhrdGZHJ5mY@yury-ThinkPad/ > > > 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: > > cpulist_read(...) > { > 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. thanks, greg k-h