Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: "Robin Murphy" <robin.murphy@arm.com>,
	andrew.murray@arm.com, maz@kernel.org,
	linux-kernel@vger.kernel.org,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Emilio López" <emilio@elopez.com.ar>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Mike Marciniszyn" <mike.marciniszyn@intel.com>,
	"Dennis Dalessandro" <dennis.dalessandro@intel.com>,
	"Yishai Hadas" <yishaih@mellanox.com>,
	"Moni Shoua" <monis@mellanox.com>,
	"David Woodhouse" <dwmw2@infradead.org>,
	"Lu Baolu" <baolu.lu@linux.intel.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	"Tom Lendacky" <thomas.lendacky@amd.com>,
	"Mirko Lindner" <mlindner@marvell.com>,
	"Stephen Hemminger" <stephen@networkplumber.org>,
	"Jiri Pirko" <jiri@resnulli.us>,
	"Solarflare linux maintainers" <linux-net-drivers@solarflare.com>,
	"Edward Cree" <ecree@solarflare.com>,
	"Martin Habets" <mhabets@solarflare.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Eric Biederman" <ebiederm@xmission.com>,
	"Thomas Graf" <tgraf@suug.ch>,
	"Herbert Xu" <herbert@gondor.apana.org.au>
Cc: james.quinlan@broadcom.com, mbrugger@suse.com,
	f.fainelli@gmail.com, phil@raspberrypi.org, wahrenst@gmx.net,
	jeremy.linton@arm.com, linux-pci@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.con>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	"David S. Miller" <davem@davemloft.net>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Chuck Lever <chuck.lever@oracle.com>,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rdma@vger.kernel.org, iommu@lists.linux-foundation.org,
	netdev@vger.kernel.org, kexec@lists.infradead.org,
	linux-nfs@vger.kernel.org
Subject: Re: [PATCH v4 7/8] linux/log2.h: Fix 64bit calculations in roundup/down_pow_two()
Date: Thu, 12 Dec 2019 13:31:57 +0100
Message-ID: <0a3e22d627a70cb60237c811b5874b9a4413329f.camel@suse.de> (raw)
In-Reply-To: <70c6b704-a12a-fb44-e93f-a6db12ed928f@arm.com>

[-- Attachment #1: Type: text/plain, Size: 2633 bytes --]

Hi Robin,

On Thu, 2019-12-05 at 17:48 +0000, Robin Murphy wrote:
> On 03/12/2019 11:47 am, Nicolas Saenz Julienne wrote:
> > Some users need to make sure their rounding function accepts and returns
> > 64bit long variables regardless of the architecture. Sadly
> > roundup/rounddown_pow_two() takes and returns unsigned longs. It turns
> > out ilog2() already handles 32/64bit calculations properly, and being
> > the building block to the round functions we can rework them as a
> > wrapper around it.
> 
> Neat! Although all the additional ULL casts this introduces seem 
> somewhat unwelcome - I suppose the (1ULL << (ilog2(n))) makes it 
> effectively always return unsigned long long now. Might it make sense to 
> cast the return value to typeof(n) to avoid this slightly non-obvious 
> behaviour (and the associated churn)?

It might alleviate some of the churn alright but I don't think a cast is really
going to make the behaviour more obvious. Say your expression is a big mess,
you'll have to analyze it to infer the output type, keeping in mind things like
integer promotion. See this example, 'params->nelem_hint' and
'params->min_size' are u16:

	diff --git a/lib/rhashtable.c b/lib/rhashtable.c
	index bdb7e4cadf05..70908678c7a8 100644
	--- a/lib/rhashtable.c
	+++ b/lib/rhashtable.c
	@@ -950,7 +950,7 @@ static size_t rounded_hashtable_size(const struct rhashtable_params *params)

		if (params->nelem_hint)
			retsize = max(roundup_pow_of_two(params->nelem_hint * 4 / 3),
	-                             (unsigned long)params->min_size);
	+                             (unsigned long long)params->min_size);
		else
			retsize = max(HASH_DEFAULT_SIZE,
				      (unsigned long)params->min_size);

With a cast the patch will look like this:

	diff --git a/lib/rhashtable.c b/lib/rhashtable.c
	index bdb7e4cadf05..70908678c7a8 100644
	--- a/lib/rhashtable.c
	+++ b/lib/rhashtable.c
	@@ -950,7 +950,7 @@ static size_t rounded_hashtable_size(const struct rhashtable_params *params)

		if (params->nelem_hint)
			retsize = max(roundup_pow_of_two(params->nelem_hint * 4 / 3),
	-                             (unsigned long)params->min_size);
	+                             (int)params->min_size);
		else
			retsize = max(HASH_DEFAULT_SIZE,
				      (unsigned long)params->min_size);

To me it's even less obvious than with a fixed ULL.

My intuition tells me to keep it as similar as the old behaviour, at the
expense of the extra churn (which is not that different from the current status
quo anyway). That said, I'll be happy to change it.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03 11:47 [PATCH v4 0/8] Raspberry Pi 4 PCIe support Nicolas Saenz Julienne
2019-12-03 11:47 ` [PATCH v4 7/8] linux/log2.h: Fix 64bit calculations in roundup/down_pow_two() Nicolas Saenz Julienne
2019-12-03 16:39   ` Chuck Lever
2019-12-04 10:56   ` Martin Habets
2019-12-04 14:17   ` Leon Romanovsky
2019-12-05  0:39   ` Lu Baolu
2019-12-05 17:48   ` Robin Murphy
2019-12-12 12:31     ` Nicolas Saenz Julienne [this message]
2019-12-05 22:30   ` Bjorn Helgaas
2019-12-12 13:16     ` Nicolas Saenz Julienne
2019-12-03 11:47 ` [PATCH v4 8/8] linux/log2.h: Use roundup/dow_pow_two() on 64bit calculations Nicolas Saenz Julienne
2019-12-03 15:53   ` Rob Herring
2019-12-03 16:06     ` Nicolas Saenz Julienne
2019-12-05 20:38   ` Bjorn Helgaas
2019-12-12 13:21     ` Nicolas Saenz Julienne

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0a3e22d627a70cb60237c811b5874b9a4413329f.camel@suse.de \
    --to=nsaenzjulienne@suse.de \
    --cc=andrew.murray@arm.com \
    --cc=anna.schumaker@netapp.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=bfields@fieldses.org \
    --cc=bhelgaas@google.com \
    --cc=chuck.lever@oracle.com \
    --cc=davem@davemloft.net \
    --cc=dennis.dalessandro@intel.com \
    --cc=dledford@redhat.com \
    --cc=dwmw2@infradead.org \
    --cc=ebiederm@xmission.com \
    --cc=ecree@solarflare.com \
    --cc=emilio@elopez.com.ar \
    --cc=f.fainelli@gmail.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=iommu@lists.linux-foundation.org \
    --cc=james.quinlan@broadcom.com \
    --cc=jeremy.linton@arm.com \
    --cc=jgg@ziepe.ca \
    --cc=jiri@resnulli.us \
    --cc=joro@8bytes.org \
    --cc=kexec@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-net-drivers@solarflare.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=mbrugger@suse.com \
    --cc=mhabets@solarflare.com \
    --cc=mike.marciniszyn@intel.com \
    --cc=mlindner@marvell.com \
    --cc=monis@mellanox.com \
    --cc=mripard@kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=netdev@vger.kernel.org \
    --cc=phil@raspberrypi.org \
    --cc=robin.murphy@arm.com \
    --cc=robin.murphy@arm.con \
    --cc=sboyd@kernel.org \
    --cc=stephen@networkplumber.org \
    --cc=tgraf@suug.ch \
    --cc=thomas.lendacky@amd.com \
    --cc=trond.myklebust@hammerspace.com \
    --cc=wahrenst@gmx.net \
    --cc=wens@csie.org \
    --cc=yishaih@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git