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 673CFE80AAD for ; Wed, 27 Sep 2023 14:23:53 +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=oJ07eUF3TcUAYT5gZ3u5TCe12EpHibSR31E0lxM9kKo=; b=tSphFqwCs3mceu tO9n/uaaYkD3EoCVgYzQ+2KNQeHr9Ovf7cv0FOwR6W91uwCYqIlxYF+F5oR7zc3KRzEAw94Z+KiNr V8cn4ICpx/JPwZSnHSoqHJDKf6PFQB9J2CAIn4UmLI2MiAiLW044eerP5H8/Qcx/BGVD/s0EgLu3k B4SlReXZdcQzog+1YwVQkEtj5Fd24viFNz+HKU8Usk948Smz1CqA5Ni+ZzgNowJJGF6SFiXkSnOkd fMpZSo35gXsG8S1+kLWM5tkksHNn+A35Ju9RfxZW00AzWhuf+zTy3+NGdcK5PF8nJl7tb2bqOO81j h/2B6b1Y6cu8zZ0nrunQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qlVRm-0017zk-0F; Wed, 27 Sep 2023 14:23:26 +0000 Received: from mail-pl1-x631.google.com ([2607:f8b0:4864:20::631]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qlVRh-0017z7-2P for linux-arm-kernel@lists.infradead.org; Wed, 27 Sep 2023 14:23:24 +0000 Received: by mail-pl1-x631.google.com with SMTP id d9443c01a7336-1c5db4925f9so82870115ad.1 for ; Wed, 27 Sep 2023 07:23:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695824600; x=1696429400; 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=t3BWG8oPBKIMl5tdWRYufTwUCrmxCw4HuyKK5y5xuEE=; b=ITAZZIeIcYb+2i4SMS2EMoUx6ygWRfsCegCKEtWhUB+Y9AXWWs5zBmnOxX9zG4x94L wCL8Fzmz7G8CfmN22vurPhHrDOuj0HfCtqz60U3I9Rnx9PymMBcKHm4hkwxmus5ojueb M7T2wsFV9dnPockl2BQnJyMwj4XIq7Aly2M8fZw0bzRHsidnphWj5gy2Ynx4eghR0z0n ViqaKFTsw/8oyvthUYEWChV+sFRyGEdXkwQa9pRO7/AIONQJOh7WHwELMlP/oN9CNDFX dDdb+pNcPggmpMvq+s+gygos8YHJi7eNxy1+Vn8pxjOhaIZFmNYQfavuK5GUu95L6mFc Xf8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695824600; x=1696429400; 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=t3BWG8oPBKIMl5tdWRYufTwUCrmxCw4HuyKK5y5xuEE=; b=SImATcKeV2FHL3il4RtISEHSOQN0JcMK2PFMhImBltngPOX1wID+Le6g3UI+HX48t8 VlXWwDcbNfoqotiy8eOfxP6IiZHDJLvpiFq1ldtVdltQ1SoUQpQmec1ACinSGJ1UqUtb 7oSLVfSXf2OgUaxTlvZFpW2b5q9CBt7OhMdAEtSq2YYqGlaqpL9hqP39QqHMR0gpx3pn WoVap+B1cb6fof8sKQ6F7mKFyZSOdOOYAl+qSQKakbcmuEvQpo+Ew7hATkIj4PlxLLXS Azjy3Tl88CL36tWV5vjzz3KRJodmKBoJA1WeYzw2iDqj8CGMPvkvikUTkuyI7+XIQkKa wi+Q== X-Gm-Message-State: AOJu0YxBoMgztLbNrLelS/XwpN5HPsgVFyWiznUK145Q3rasInwvVPEM fO7ivXgZ4p9oHtW8EdWSc34= X-Google-Smtp-Source: AGHT+IFhCa0TTPpNT0tDTforsake3LWuAPPG9NQAtYiLDzkLyIHq0/HcXziEzkT30AxGecMY3lFUew== X-Received: by 2002:a17:902:ecca:b0:1c3:432f:9f69 with SMTP id a10-20020a170902ecca00b001c3432f9f69mr3809052plh.23.1695824600466; Wed, 27 Sep 2023 07:23:20 -0700 (PDT) Received: from sol ([118.209.204.34]) by smtp.gmail.com with ESMTPSA id jw12-20020a170903278c00b001c625acfed0sm5411055plb.44.2023.09.27.07.23.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Sep 2023 07:23:20 -0700 (PDT) Date: Wed, 27 Sep 2023 22:23:12 +0800 From: Kent Gibson To: Andy Shevchenko Cc: Linus Walleij , Bartosz Golaszewski , Yury Norov , 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 5/5] gpiolib: cdev: Utilize more bitmap APIs Message-ID: References: <20230926052007.3917389-1-andriy.shevchenko@linux.intel.com> <20230926052007.3917389-6-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-20230927_072321_810706_66A8A07E X-CRM114-Status: GOOD ( 51.90 ) 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 04:59:34PM +0300, Andy Shevchenko wrote: > On Wed, Sep 27, 2023 at 09:49:35PM +0800, Kent Gibson wrote: > > On Wed, Sep 27, 2023 at 03:17:06PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 27, 2023 at 09:32:11AM +0800, Kent Gibson wrote: > > > > On Tue, Sep 26, 2023 at 08:20:07AM +0300, Andy Shevchenko wrote: > > > > > Currently we have a few bitmap calls that are open coded in the library > > > > > module. Let's convert them to use generic bitmap APIs instead. > > > > > > > > Firstly, I didn't consider using the bitmap module here as, in my mind at > > > > least, that is intended for bitmaps wider than 64 bits, or with variable > > > > width. In this case the bitmap is fixed at 64 bits, so bitops seemed more > > > > appropriate. > > > > > > > > And I would argue that they aren't "open coded" - they are parallelized > > > > to reduce the number of passes over the bitmap. > > > > This change serialises them, e.g. the get used to require 2 passes over > > > > the bitmap, it now requires 3 or 4. The set used to require 1 and now > > > > requires 2. > > > > And there are additional copies that the original doesn't require. > > > > So your change looks less efficient to me - unless there is direct > > > > hardware support for bitmap ops?? > > > > > > > > Wrt the argument that the serialized form is clearer and more > > > > maintainable, optimised code is frequently more cryptic - as noted in > > > > bitmap.c itself, and this code has remained unchanged since it was merged > > > > 3 years ago, so the only maintenance it has required is to be more > > > > maintainable?? Ok then. > > > > > > > > Your patch is functionally equivalent and pass my uAPI tests, so > > > > > > > > Tested-by: Kent Gibson > > > > > > Thanks for testing! > > > > Not a problem - that is what test suites are for. > > > > > > but my preference is to leave it as is. > > > > > > As Yury mentioned we need to look at bitmap APIs and make them possible to have > > > a compile-time optimizations. With that in mind, I would prefer bitmap APIs > > > over open-coded stuff which is hardly to be understood (yes, I still point > > > out that it takes a few hours to me, maybe because I'm stupid enough, to > > > get what's the heck is going one there, esp. for the == 1 case). > > > > Really? With all the bits out in the open it seems pretty clear to me. > > Clearer than scatter/gather in fact. > > Yes, you are biased. :-) Ask some stranger about this code and I am pretty sure > there will be double-figures percentage of people who can tell that the current > code is a bit voodoo. > It is the same as yours - just inside out. i.e. it performs the ops per selected line, not each op on the whole bitmap of lines. > > Sure, if there is suitable hardware support then bitmaps COULD be faster > > than bitops. But without that, and that is the general case, it will be > > slower. Do you have ANY cases where your implementation is currently > > faster? Then you would have a stronger case. > > Why do we care here about performance? But if we do, I would check this on > the 32-bit platform where 64-bit operations somewhat problematic / slow. > Yet you argue that bitmaps could be more performant?? Pick a side! > If Yury gives an idea about performance tests I can consider to add this > piece to compare with and we might see the difference. > > > And if you find the existing implementation unclear then the appropriate > > solution is to better document it, as bitmaps itself does, not replace it > > with something simpler and slower. > > Documentation will be needed either way. In general statistics it will be 50/50 > who (mis)understands this or new code. Pity that the original author of the code > hadn't though about documenting this... > And who was the original author? I forget. What you mean to say is it is a pity the reviewers at the time were satisfied with the code as it stands, right? Cos there is a process here. As I recall reviewers were more often than not complaining about pointless comments, not the lack of comments, so the natural bias as the author is towards under-documenting... > > > Yet, it opens a way to scale this in case we might have v3 ABI that let's say > > > allows to work with 512 GPIOs at a time. With your code it will be much harder > > > to achieve and see what you wrote about maintenance (in that case). > > > > v3 ABI?? libgpiod v2 is barely out the door! > > Do you have any cases where 64 lines per request is limiting? > > IIRC it was SO question where the OP asks exactly about breaking the 64 lines > limitation in the current ABI. > > > If that sort of speculation isn't premature optimisation then I don't know > > what is. > > No, based on the real question / discussion, just have no link at hand. > But it's quite a niche, I can agree. > Let me know if you find a ref to that discussion - I'm curious. Cheers, Kent. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel