Received-SPF: pass (google.com: domain of linux-kernel+bounces-211076-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Wed, 12 Jun 2024 10:28:06 +0300 (EEST)
To: Shravan Ramani <shravankr@nvidia.com>
cc: Hans de Goede <hdegoede@redhat.com>, Vadim Pasternak <vadimp@nvidia.com>, 
    David Thompson <davthompson@nvidia.com>, 
    "platform-driver-x86@vger.kernel.org" <platform-driver-x86@vger.kernel.org>, 
    LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/4] platform/mellanox: mlxbf-pmc: Add support for
 64-bit counters and cycle count
In-Reply-To: <DM4PR12MB5136EAD83A50869388E96FF3C0C72@DM4PR12MB5136.namprd12.prod.outlook.com>
Message-ID: <370b5e44-cf92-21af-8c01-dbb208bf323f@linux.intel.com>
References: <cover.1716205838.git.shravankr@nvidia.com> <ce077a0db5d4afdbcc63a635fece9793aaae055f.1716205838.git.shravankr@nvidia.com> <70d3c0af-8bf6-2e33-074d-5b1719a5674f@linux.intel.com> <DM4PR12MB513695D2BE98AA46A95B4C60C0FF2@DM4PR12MB5136.namprd12.prod.outlook.com>
 <33f25d4f-386c-6df6-344d-8b7aa011e69c@linux.intel.com> <DM4PR12MB5136EAD83A50869388E96FF3C0C72@DM4PR12MB5136.namprd12.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Tue, 11 Jun 2024, Shravan Ramani wrote:

> > > 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.

I know all this by now, but we're discussion here is whether kernel should 
do "special handling". Although, it's not really correct to depict 
representing 64-bit counter in its entirety as "special handling".

I think the kernel should combine the 64-bit halved and you argumented 
it shouldn't. When I went to confirm the claim your argument was based 
on, I couldn't find on what basis the claim was made.

> 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.

This does nothing to answer my question. Where in the kernel, there's an 
example where a 64-bit counter for BlueField platform is presented as 2 
32-bit counters? If there isn't any examples in the kernel, your statement 
about consistency within the platform doesn't hold water, quoted (again) 
here for clarity what I'm refering to:

"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."

Where I can find those "other interfaces" that already follow this 
convention?

-- 
 i.