Message-ID:
 <DM4PR12MB5136EAD83A50869388E96FF3C0C72@DM4PR12MB5136.namprd12.prod.outlook.com>
Date: Tue, 11 Jun 2024 13:34:35 +0000
From: Shravan Ramani <shravankr@...dia.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: Hans de Goede <hdegoede@...hat.com>, Vadim Pasternak <vadimp@...dia.com>,
	David Thompson <davthompson@...dia.com>,
	"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] platform/mellanox: mlxbf-pmc: Add support for
 64-bit counters and cycle count

> > When 2 32-bit counters are coupled to form a 64-bit counter using this setting,
> > one counter will hold the lower 32 bits while the other will hold the upper 32.
> > So the other counter (or syses corresponding to it) also needs to be accessed.
> >
> > > For 64-bit counter, I suppose the userspace is expected to read the full
> > > counter from two sysfs files and combine the value (your documentation
> > > doesn't explain this)? That seems non-optimal, why cannot kernel just
> > > return the full combined 64-value directly in kernel?
> > 
> > I will add more clear comments for this.
> > While it is true that the driver could combine the 2 fields and present a
> > 64-bit value via one of the sysfs, the reason for the current approach is that
> > there are other interfaces which expose the same counters for our platform
> > and there are tools that are expected to work on top of both interfaces for
> > the purpose of collecting performance stats.
>
> > The other interfaces follow this
> > approach of having lower and upper 32-bits separately in each counter, and
> > the tools expect the same. Hence the driver follows this approach to keep
> > things consistent across the BlueField platform.
>
> Hi,
>
> I went to look through the existing arrays in mlxbf-pmc.c but did not find
> any entries that would have clearly indicated the counters being hi/lo
> parts of the same counter. There were a few 0/1 ones which could be the
> same counter although I suspect even they are not parts of the same
> counter but two separate entities called 0 and 1 having the same counter.
>
> Could you please elaborate further what you meant with the note about
> other interfaces above so I can better assess the claim?

When combining 2 counters using the "use_odd_counter" setting, the mechanism
of joining them or assigning upper or lower 32 bits to a counter is handled in HW
and not by the driver. For example, if bit0 of "use_odd_counter" is set, counter0
and counter1 (which were originally separate counters) automatically become
the lower and upper bits of one 64-bit value. The user needs to read both these
sysfs separately to get the full 64-bit value. The driver does not do any special
handling for such cases, merely provides access to both counter0 and counter1.

Since the events supported by the blocks are quite HW centric and low-level in
nature, the driver is generally used alongside various tools which work on top of
this driver to collect telemetry info and provide more readable statistics to the
end-user. Similar to this driver, there are other FW interfaces providing access to
these counters (same and other additional ones as well that belong to other HW
blocks). For the sake of consistency and to allow the tools to be compatible with
all interfaces, the counter data needs to be accessible in the same way, ie, as 32-bit
upper and lower values in counter0 and counter1 sysfs as in the above case.

Thanks,
Shravan

Powered by blists - more mailing lists