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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 34048E784AB for ; Mon, 2 Oct 2023 04:07:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xd/NvVRr6LR8dIy60Haf6+yrGNq6mXXz6IgXzKWodnM=; b=gapay7WEjBWMLx 9DA623Mcm3nr0a5ncrvSbf2+5mCfmQZtn8Wd8yModp7QtBN6YFScW0y9dFbNHumIHc9h6976d3KyN 1zFpdbxfPYhPP2HgPP4Q4eFvAG+w/PjZTNp6lCo4ZCuk0w4YLh3KxtoNHp9boMSeHC8R47QgrxllU AK2ZJ4hZGr5xQUzVQnAGR/1gcCObi9oaVA2Haa/1vqWwKpurwHLUqDMlj3pV8JngbdRK8QGDJ4ITb f+XVjQt9gZTPv4sZqwkmdliFbkEtElazAzIBcTz0xOu+Dd9dXBChuNaLOloDqwll0Xv42YngxFI54 uWnllGvPN22BBqHHDeGQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qnACX-00Bwel-0j; Mon, 02 Oct 2023 04:06:33 +0000 Received: from mail-vk1-xa32.google.com ([2607:f8b0:4864:20::a32]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qnACU-00BweO-0K for linux-arm-kernel@lists.infradead.org; Mon, 02 Oct 2023 04:06:31 +0000 Received: by mail-vk1-xa32.google.com with SMTP id 71dfb90a1353d-49040dc5cedso6027946e0c.3 for ; Sun, 01 Oct 2023 21:06:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1696219587; x=1696824387; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=a7N14C5lw8nxoWs6YpUUkEHECVFntWCWWPn4SACIIVI=; b=N4o3sSTusf9dlxNROhP5kXrtf+wyK+3i73u28xOPSljA372xSHG/QttUdrSn36GOfs 6EQV8FYLiDZwFW+UKG/tGAmsszwTUlqcv4UqZ2MsVyh3Zh4Tos5Hw73zFW20bA4u5boL hdsvQoyQ0jz+oezO5296Dsbxk0XqRiocqOipu974SL/6FN9H0IN4h35gfbXKQU2OLMVQ m9fzNFl/uHKguGVvBzCmLZY9Ftvc2HD/iRXS8q83nNgxsn3qwTxGVj+XDEsxKOPk87ZO oVrSjTndFAKMlc4CLpRWodbrnU7pfeD6AazPAC0ZapI0mTFAVZsf4jFJXYFeoIEX48oU pFYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696219587; x=1696824387; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=a7N14C5lw8nxoWs6YpUUkEHECVFntWCWWPn4SACIIVI=; b=rNsycPQhKHt+S1Qy3wcOPaSx9TAA/w8w+2Tmc8nvPhbUf4Nz9LbS1PtNK7oDYNjFkh 3NdWTdqFtg9uCjTt8WUbCHviVATH6h4vVs8KncEAffVmLpjEhd6E6iVZJn8+i//ANbGY Cly2fXruIBFS8O6GQb0aA4a3m4PV6tKfqS+eluIz7k2gsqOoRaR/Bu3NIkw864iZ11pU g2fxC5aYGujhYPJl6BTyJILAMptAgTHGOxbO8FWCJ6v9euBdr3cdO6RPRTvB/mZsXyF2 zuZomjR5Ky8EWHU8pKEkhtWM1qwnqiIs7bqCg6ndbFbD/xQbjUvc1/xlNlfI1IINzw0y HrCA== X-Gm-Message-State: AOJu0YxckbmYp7ZxCBgUC2XUcBJyrwkq5gQGOAPmgaKIIrKObvRiqAzI 2TNxqtgNseCXAFR81tdFn+Y= X-Google-Smtp-Source: AGHT+IHpyA9ox0N97+86UDd7xs50XCnM7RzezJ8RYrQ9jbNn5sUxvlv6Q81SiiShZQVQsb/3chitIw== X-Received: by 2002:a1f:e3c4:0:b0:49a:b737:4df7 with SMTP id a187-20020a1fe3c4000000b0049ab7374df7mr6283589vkh.5.1696219586025; Sun, 01 Oct 2023 21:06:26 -0700 (PDT) Received: from localhost ([2607:fb90:beca:c7a7:dba8:3746:709f:6151]) by smtp.gmail.com with ESMTPSA id f24-20020ac5c9b8000000b0049d20faf953sm800101vkm.55.2023.10.01.21.06.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 01 Oct 2023 21:06:25 -0700 (PDT) Date: Sun, 1 Oct 2023 21:06:24 -0700 From: Yury Norov To: Andy Shevchenko Cc: Linus Walleij , Bartosz Golaszewski , linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Shubhrajyoti Datta , Srinivas Neeli , Michal Simek , Bartosz Golaszewski , Rasmus Villemoes , Marek =?iso-8859-1?Q?Beh=FAn?= Subject: Re: [PATCH v1 2/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers Message-ID: References: <20230926052007.3917389-1-andriy.shevchenko@linux.intel.com> <20230926052007.3917389-3-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231001_210630_182000_929D89B9 X-CRM114-Status: GOOD ( 32.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 27, 2023 at 03:02:34PM +0300, Andy Shevchenko wrote: [...] > > It looks like those are designed complement to each other. Is that > > true? If so, can you make your example showing that > > scatter -> gather -> scatter > > would restore the original bitmap? > > It looks like you stopped reading documentation somewhere on the middle. What a wonderful week of strong statements... Whatever... > The two APIs are documented with the same example which makes it clear > that they are data-loss transformations. > > Do you need something like this to be added (in both documentations): > > The bitmap_scatter(), when executed over the @dst bitmap, will > restore the @src one if the @mask is kept the same, see the example > in the function description. > > ? > > > If I'm wrong, can you please underline that they are not complement, > > and why? > > No, you are not. I should be confused even more. You're saying that I'm not wrong here, and few lines above you're saying it's a data loss... I don't mind this new 3-liners, but I'd like you to have a well better wording and testing around them because those bitmap_scatter/gather are all about performance, readability and usability. To begin with, the whole name of the series: "get rid of bitmap_remap() and bitmap_biremap() uses" is wrong because the functions are still here, and are still used. Even worse, instead of getting rid of some useless code, you're bloating the kernel with something that duplicates existing functionality. This is an example of a series that 'gets rid of' something for true: https://yhbt.net/lore/all/20230925023817.782509-7-yury.norov@gmail.com/T/ (And unfortunately it's still unreviewed.) But I understand your motivation, and as I already said, I like this series in general. So let's please figure out a better wording before moving forward? Below are some my of thought. 1. Stop saying that you're getting rid of something when you're not. I'd suggest something like: "add simple and verbose alternatives to bitmap_remap(), and use them where appropriate". 2. Don't blame people considering a parameter named 'mask' as a mask. I mean this sentence: > You should get the mask meaning. It's not the bit provider, it's a bit > positions provider. If it's a 'bit position provider', please give it a proper name, for example 'pos'. I'd suggest something like: unsigned long bitmap_scatter(unsigned long *dst, unsigned long *pos, unsigned long *val) 3. If you're saying that new API is a simplified replacement for something, I'd like to see the full contract, i.e. explicit list of all simplifications and limitations implied: - val == dst is not handled; - when 'pos' is empty, val is not copied to dst; - new API doesn't wrap around 0, like bitmap_remap() does; - set bits in 'val' are not copied to 'dst' when not in 'pos' (?)' - anything else else? 4. Similarly to previous item, I'd like to have explicit understanding and examples where and how bitmap_remap may be replaced. You're only saying that it is possible when either 'new' or 'old' are dense. Is that the only case? Can you add a test that explicitly checks that bitmap_remap and bitmap_scatter/gather produce the same output. Something like this: bitmap_remap(dst1, val, dense_map, pos, nbits); bitmap_scatter(dst2, val, pos, nbits); check_eq_bitmap(dst1, dst2, nbits); 5. Can you add a picture like this to explain the algorithm even more: mask: ...v..vv..v...vv bits: 0000001100000010 1. ^ ^^ ^ 0 2. | || | 10 3. | || +> 010 4. | |+--> 1010 5. | +--> 11010 6. +----> 011010 gather: ..........011010 5. Regarding my confusion, I had to draw the picture above to realise how it's possible that scatter/gather are inverse and data-loss (i.e. not inverse) at the same time. Can you explain it with a wording like this: "For bits selected by 'pos' bitmap, gathering a 'val' bitmap with the following scattering restores the original map. All other bits values are lost and replaced with zeros." Similarly for gathering. And please add a test case. 6. Regarding performance. I think it's wrong to say that that your code is better optimized then some other, and then ask your reviewers to figure out how to measure the difference. If you make such statement, you should already have some test or real-life measurement. However, if you ask me, I can suggest you to pull this patch: https://lore.kernel.org/lkml/20230828184353.5145-4-yury.norov@gmail.com/ and modify/extend it in a way that both bitmap_remap and bitmap_scatter/gather take the same 'val' and 'pos' bitmaps, produce the same output, and then see which code is faster. Worth to mention that since all current users of your API are working on 64-bit maps, performance is doubty an issue. So you can drop the 'optimization' part of your wording, and don't add performance test. Thanks, Yury _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel