Re: [PATCH v4 09/19] x86/resctrl: Initialize ABMC counters bitmap

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Babu,

On 6/19/24 10:03 AM, Moger, Babu wrote:
On 6/13/24 20:42, Reinette Chatre wrote:
On 5/24/24 5:23 AM, Babu Moger wrote:

...

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3071bbb7a15e..400ae405e10e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
       return !test_bit(closid, &closid_free_map);
   }
   +/*
+ * ABMC Counter bitmap and length for tracking available counters.
+ * ABMC feature provides set of hardware counters for enabling events.
+ * Each event takes one hardware counter. Kernel needs to keep track
+ * of number of available counters.
+ */
+static unsigned long num_cntrs_free_map;

Why does variable have "num" in its name? That seems strange. How
about just "mon_cntrs_free_map

It came from patch 4/19.

struct resctrl_mon {
         int                     num_rmid;
+       int                     num_cntrs;
         struct list_head        evt_list;
  };

num_cntrs_free_map is a bitmap representing num_cntrs. Kept the matching
name for better understanding. Renaming it will loose that connection.

I disagree. The "num" in "num_cntrs" indicates that this variable stores
the _number_ of a particular entity. In this case "cntrs" or ... counters.
This is just like how resctrl uses "num_closid" to indicate how many closid
are available and then have a separate "closid_free_map" to actually track
now closids are used ... it is _not_ "num_closid_free_map". Similarly,
"num_rmid" indicates how many RMID are available and the "rmid_free_lru"
tracks how RMID are used ... it is _not_ "num_rmid_free_lru".

If I rename then I will have to rename both.

No, you do not.


How about mbm_cntrs and mbm_cntrs_free_map?

"mbm_cntrs" does sound good. It is more specific than "cntrs". I would suggest that
use "num_mbm_cntrs" to match with "num_rmid" and "num_closid" and then
you can introduce "mbm_cntrs_free_map".

+static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

Only reason is, it is supposed to be unsigned.  I can change it "unsigned
int".


+
+static void num_cntrs_init(void)

mon_cntrs_init() ?

mbm_cntrs_init?

Sounds good.



+{
+    struct rdt_resource *r =
&rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+
+    bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
+    num_cntrs_free_map_len = r->mon.num_cntrs;
+}
+
   /**
    * rdtgroup_mode_by_closid - Return mode of resource group with closid
    * @closid: closid if the resource group
@@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum
resctrl_res_level l, bool enable)

resctrl_abmc_set_all() was initially created as a complement of
resctrl_abmc_set_one() ... but with more initialization added to
resctrl_abmc_set_all() this relationship becomes vague.

Yes. Understood. Let me know if want me to change anything here.

How about renaming resctrl_abmc_set_all() to _resctrl_abmc_enable()?

Reinette




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux