[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<SN6PR02MB4157C2266EB06759A8E16BD1D495A@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sat, 24 Jan 2026 00:44:50 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>, Stanislav Kinsburskii
<skinsburskii@...ux.microsoft.com>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kys@...rosoft.com" <kys@...rosoft.com>, "haiyangz@...rosoft.com"
<haiyangz@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
"decui@...rosoft.com" <decui@...rosoft.com>, "longli@...rosoft.com"
<longli@...rosoft.com>, "prapal@...ux.microsoft.com"
<prapal@...ux.microsoft.com>, "mrathor@...ux.microsoft.com"
<mrathor@...ux.microsoft.com>, "paekkaladevi@...ux.microsoft.com"
<paekkaladevi@...ux.microsoft.com>
Subject: RE: [PATCH v4 6/7] mshv: Add data for printing stats page counters
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Friday, January 23, 2026 4:13 PM
>
> On 1/23/2026 2:31 PM, Stanislav Kinsburskii wrote:
> > On Fri, Jan 23, 2026 at 11:04:52AM -0800, Nuno Das Neves wrote:
> >> On 1/23/2026 9:09 AM, Michael Kelley wrote:
> >>> From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Wednesday, January 21, 2026 1:46 PM
> >>>>
> >>>> Introduce hv_counters.c, containing static data corresponding to
> >>>> HV_*_COUNTER enums in the hypervisor source. Defining the enum
> >>>> members as an array instead makes more sense, since it will be
> >>>> iterated over to print counter information to debugfs.
> >>>
> >>> I would have expected the filename to be mshv_counters.c, so that the association
> >>> with the MS hypervisor is clear. And the file is inextricably linked to mshv_debugfs.c,
> >>> which of course has the "mshv_" prefix. Or is there some thinking I'm not aware of
> >>> for using the "hv_" prefix?
> >>>
> >> Good question - I originally thought of using hv_ because the definitions inside are
> >> part of the hypervisor ABI, and hence also have the hv_ prefix.
> >>
> >> However you have a good point, and I'm not opposed to changing it.
> >>
> >> Maybe to just be super explicit: "mshv_debugfs_counters.c" ?
> >>
> >
> > This is reudnant from my POV.
> > If these counters are only used by mshv_debugfs.c, then should rather be
> > a part of this file.
> > What was the reason to move them elsewhere?
> >
>
> Just a matter of taste - so there isn't ~450 lines of definitions at the beginning of
> mshv_debugfs.c. But I'm not fussed. If you think it's better to just prepend the
> definitions to mshv_debugfs.c, then that's an easy change.
>
> Nuno
FWIW, I preferred the separate file so that the main debugfs code
isn't burdened with 450 lines of definitions that aren't going to be
edited/revised/improved via the typical processes. The current
mshv_debugfs.c is a reasonable 700 lines of code without all the
definitions.
But it's not a big deal for me either way.
Michael
>
> > Thanks,
> > Stanislav
> >
> >>> Also, I see in Patch 7 of this series that hv_counters.c is #included as a .c file
> >>> in mshv_debugfs.c. Is there a reason for doing the #include instead of adding
> >>> hv_counters.c to the Makefile and building it on its own? You would need to
> >>> add a handful of extern statements to mshv_root.h so that the tables are
> >>> referenceable from mshv_debugfs.c. But that would seem to be the more
> >>> normal way of doing things. #including a .c file is unusual.
> >>>
> >>
> >> Yes...I thought I could avoid noise in mshv_root.h and the Makefile, since it's
> >> only relevant for mshv_debugfs.c. However I could see this file (whether as .c or
> >> .h) being misused and included elsewhere inadvertantly, which would duplicate the
> >> tables, so maybe doing it the normal way is a better idea, even if mshv_debugfs.c
> >> is likely the only user.
> >>
> >>> See one more comment on the last line of this patch ...
> >>>
>
> <snip>
Powered by blists - more mailing lists