Received-SPF: pass (google.com: domain of linux-kernel+bounces-106222-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 18 Mar 2024 14:29:23 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
CC: David Lechner <dlechner@baylibre.com>, Jonathan Cameron
	<jic23@kernel.org>, Michael Hennerich <michael.hennerich@analog.com>, Nuno
 =?ISO-8859-1?Q?S=E1?= <nuno.sa@analog.com>, <linux-iio@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] iio: adc: ad7944: Add support for "3-wire mode"
Message-ID: <20240318142923.000042f4@Huawei.com>
In-Reply-To: <CAHp75VeQcvuEy4V6-+3PeWTZJ9=Qae0AiiNB93OOw3wuc-uh3A@mail.gmail.com>
References: <20240314-mainline-ad7944-3-wire-mode-v2-1-d469da0705d2@baylibre.com>
	<ZfX5jynjW4M9pvw1@surfacebook.localdomain>
	<20240318124041.0000032d@Huawei.com>
	<CAHp75VeQcvuEy4V6-+3PeWTZJ9=Qae0AiiNB93OOw3wuc-uh3A@mail.gmail.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, 18 Mar 2024 15:09:32 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Mar 18, 2024 at 2:41=E2=80=AFPM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > > >  struct ad7944_adc {
> > > >     struct spi_device *spi;
> > > > +   enum ad7944_spi_mode spi_mode;
> > > >     /* Chip-specific timing specifications. */
> > > >     const struct ad7944_timing_spec *timing_spec;
> > > >     /* GPIO connected to CNV pin. */
> > > > @@ -58,6 +75,9 @@ struct ad7944_adc {
> > > >      } sample __aligned(IIO_DMA_MINALIGN);
> > > >  }; =20
> > >
> > > Have you run `pahole` to see if there is a better place for a new mem=
ber? =20
> >
> > I know this matters for structures where we see lots of them, but do we=
 actually
> > care for one offs?  Whilst it doesn't matter here I'd focus much more
> > on readability and like parameter grouping for cases like this than was=
ting
> > a few bytes. =20
>=20
> This is _also_ true, but think more about cache line contamination.
> Even not-so-important bytes may decrease the performance. In some
> cases it's tolerable, in some it is not (high-speed ADC). In general I
> assume that the developer has to understand many aspects of the
> software and cache line contamination may be last but definitely not
> least.
>=20

Not totally sure what you are covering with contamination as many aspects
around caches and that's not really a standard term for any of them (as
far as I know).

It's part of a multi cacheline allocation anyway (because it's tacked on the
end of the iio device struct, so fairly unlikely to share with other alloca=
tions
and definitely not on ARM because of the trailing __aligned(IIO_DMA_MINALIG=
N)
elements.

If it matters more locally, then pahole is more likely to push you to pack
things together in a fashion that makes false sharing and similar perf issu=
es
more likely if you are grouping things for packing purposes rather than
logical groups.

If you just mean cache pressure then fair enough if we squeeze everything i=
nto
one cacheline and that doesn't cause false sharing.
'Maybe' this will fit on x86. On Arm64 it's not going to
make any difference, just moving the padding around a bit within the line.

So I'd argue premature optimization for a small, one off, structure.

Jonathan