Received: by 2002:a05:6a10:1287:0:0:0:0 with SMTP id d7csp707095pxv; Thu, 22 Jul 2021 10:11:57 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwO9Aziab8+wA4lRHmkuvibM9+EdIqdinIlOn2W64NMS5VL17wg94XTlkS+nSAlOdU9aoGo X-Received: by 2002:a17:906:f746:: with SMTP id jp6mr888477ejb.39.1626973916994; Thu, 22 Jul 2021 10:11:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1626973916; cv=none; d=google.com; s=arc-20160816; b=ZH8UfcS2MwHeyMC/ukoN3rnwvA1ctuifFXX6l67ih57F21dN5mlQgBw4tfQgEPOt2S oY5rFsjrQR2bbdh7vGMpVW9lNVdq6L/pCTAm8u9MWO+Vr9utzdmshB5+PVE1pEQPwLUb NY/Gx6krVtLU2bCmxdnVlZh/buHaY3ZkUoWhoQ2a12xpKd1J4aK2pt6a3CHCNWu81uoz soUsUws/8HOgE+KRVVLuQJyYvhSheJdTX0Zlr/8rjT8e5YiL9bThUDBYZtBCs0dhpil0 UVdpu4C57UuMfRlUPmFxWEHW194dFFP2eklcXNCqk1KTgYf54zVkRe4NSehstQjqIgnW 6G7A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=0KWjIUvssejxul/NQJNnk46DY8y9cdn4iweGJV3mrps=; b=KWi+8s3F6d1oxVEUpRDoB8izGQvbfQNCJzaQQQw5/l3oEHShNWDN2qZ/s4ajVyB9ie wYW8lIxIy+m3abx6lzAMexB0kow/32K1NKNqmCv4PSwaFIDBZv8jSuEIabppXb7cghRB c6nvlyTK7qyTc9FjPkDIV5omndvwBTQSWKutPj/g4NpElU8LeNTv/cZ2OFpXefxUFeKS I93fOefg+u9jpiRnP3A04A9cZ49idSZMwVGje82T1LqLY6jmDjhgJGcYZGjFTeBK+nZ9 2Q597fQifztT/1b7Up3VwrEDy7uV7hdB8tOyTU/WfvXBLXN5IFXy0zuboF1ZwaNO03dq 5kvg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=klGEArfP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id n1si31794492edy.32.2021.07.22.10.11.32; Thu, 22 Jul 2021 10:11:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=klGEArfP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229806AbhGVQ25 (ORCPT + 99 others); Thu, 22 Jul 2021 12:28:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54910 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229731AbhGVQ2z (ORCPT ); Thu, 22 Jul 2021 12:28:55 -0400 Received: from mail-qv1-xf31.google.com (mail-qv1-xf31.google.com [IPv6:2607:f8b0:4864:20::f31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42550C061760 for ; Thu, 22 Jul 2021 10:09:30 -0700 (PDT) Received: by mail-qv1-xf31.google.com with SMTP id w6so48056qvh.3 for ; Thu, 22 Jul 2021 10:09:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=0KWjIUvssejxul/NQJNnk46DY8y9cdn4iweGJV3mrps=; b=klGEArfPT2JVo45FJ+D2h4cg5wMUxNv82s/Ugl9kYmY95SCzz/8N25LXSit+yQ909v Ndexb7rWUo6cSGqEx+PUD6X/ViBTphGVb9G4JdIWDuc9Xr9BAHsy53fhM1fzAE6pQzxe ndjsOjFaXq0/17MeIY6p3Fc4bJkPjtzE1ADCHruywhhbUzzGbSvx10e2LRP3N879Ca6W VTbZdV9/G6hIXyUsgVltCqGmK0r3wJ1QFIJKg2CFqF2uDzJ5KjlXTNO3p5nYsTyoZ+gt KGfo/FCtl7fB7pUR01uGDnF8ZD+2ZOjqavDiupsYw76eafzWurT0riwERqNlBTdH4DzJ CJQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=0KWjIUvssejxul/NQJNnk46DY8y9cdn4iweGJV3mrps=; b=N8XKqwtj3AJkClNWDu9b+d7brOktbtkVqGRzqXXCSq53Y43tiSW8OCIijfoIGfn/ga 9t+QMTHWEeM0kZNP9qfg0+qQYNPy5Z1VrVvmAzitjjPQaYjclXobz6zMvLJKBve80nPc 95I3UBf8BLquAGJWkRKbs//A28ghoaxmlOYKWG8BAzqxCyeEjp+kePO8kdG8cqU1ZS3D WPxpKfYlTNjdk6jty4RdRyec0hSI4M3pEr5eHI38dpn6aJM+twVtRDdL67ncYo/3G3l+ oFhwKFOOuXzJVs8qnTzik+g5rPi2vdN5VERRPBKxTGWgJBdzBzC12yl4jX5YYEf2vEt0 RIuA== X-Gm-Message-State: AOAM533eetCtm/f3ELs13MD/GfCEGWsLoorKbPe86den3VKq9d627feh +Xcba2j/PMglShMuGxtT0Vg= X-Received: by 2002:a0c:f152:: with SMTP id y18mr686690qvl.4.1626973769301; Thu, 22 Jul 2021 10:09:29 -0700 (PDT) Received: from localhost ([12.28.44.171]) by smtp.gmail.com with ESMTPSA id i123sm11483934qkf.60.2021.07.22.10.09.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Jul 2021 10:09:28 -0700 (PDT) Date: Thu, 22 Jul 2021 10:09:27 -0700 From: Yury Norov To: Greg Kroah-Hartman 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 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? > 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. > 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. If it's completely impossible to store a pointer, and the bug is real and urgent, then the best solution I see is to move bitmap_get_print_buf() to the driver/base/node.c code, because it's not optimal for general use. Which I already suggested here: https://lkml.org/lkml/2021/7/16/763 Can you or Barry tell, would one of those alternative solutions work for you? Thanks, Yury