Received-SPF: pass (google.com: domain of linux-kernel+bounces-213671-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240613082449.197397-1-ubizjak@gmail.com> <20240613155134.GA1062951@bhelgaas>
In-Reply-To: <20240613155134.GA1062951@bhelgaas>
From: Uros Bizjak <ubizjak@gmail.com>
Date: Thu, 13 Jun 2024 18:44:36 +0200
Message-ID: <CAFULd4YmckrG1RBzSnhtNCWmmLmU4JhXOxwBGOLdrOu=FWLOuA@mail.gmail.com>
Subject: Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bjorn Helgaas <bhelgaas@google.com>, Lukas Wunner <lukas@wunner.de>, 
	=?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 13, 2024 at 5:51=E2=80=AFPM Bjorn Helgaas <helgaas@kernel.org> =
wrote:
>
> [+cc Lukas, Ilpo]
>
> On Thu, Jun 13, 2024 at 10:24:24AM +0200, Uros Bizjak wrote:
> > Use atomic_{fetch_}andnot(i, v) instead of atomic_{fetch_}and(~i, v).
>
> If the purpose is to improve readability, let's mention that here.
> Since this only touches pciehp, make the subject line "PCI: pciehp:
> ..." as was done in the past.
>
> It looks like every use of atomic_and() uses a ~value and is hence a
> candidate for a similar change, but I'm not sure that converting to
> "andnot" and removing the explicit bitwise NOT is really a readability
> benefit.
>
> If it were named something like "atomic_clear_bits", I'd be totally in
> favor since that's a little higher-level description, but that ship
> has long since sailed.

FYI, the set of atomic primitives and their corresponding names have
quite a long history. These are based on IA-64 psABI [1, section 7.4]
when this particular primitive was named
__sync_nand_and_fetch/__sync_fetch_and_nand. Even GCC got the and-not
part wrong (it implemented it as not-and in some ancient version), so
luckily the kernel named it atomic_{fetch_}andnot.

As far as the patch is concerned, some architectures emit atomic
andnot instruction. In the proposed patch, we have a constant argument
to atomic_and, and the compiler is smart enough to apply the bitwise
not to the argument and emits an inverted constant argument. So, in
reality, the same code is produced.

x86 with BMI1 ISA extension provides ANDN instruction, but it can't be
used with LOCK prefix.

So, if there is no readability benefit, it is OK with me to drop the patch.

[1] https://refspecs.linuxfoundation.org/elf/IA64-SysV-psABI.pdf

Thanks,
Uros.