Hi Tony,
In subject:
closid_num_dirty_rmid -> closid_num_dirty_rmid[]
On 8/29/25 12:33 PM, Tony Luck wrote:
> closid_num_dirty_rmid[] is allocated in dom_data_init() during resctrl
> initialization and freed by dom_data_exit() during resctrl exit giving
> it the same life cycle as rmid_ptrs[].
>
> Move closid_num_dirty_rmid[] allocaction/free out to
> resctrl_l3_mon_resource_init() and resctrl_l3_mon_resource_exit() in
> preparation for rmid_ptrs[] to be allocated on resctrl mount in support
> of the new telemetry events.
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> fs/resctrl/monitor.c | 56 ++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 33432a7f56da..d5b96aca5d03 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -805,36 +805,14 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
> static int dom_data_init(struct rdt_resource *r)
> {
> u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> - u32 num_closid = resctrl_arch_get_num_closid(r);
> struct rmid_entry *entry = NULL;
> int err = 0, i;
> u32 idx;
>
> mutex_lock(&rdtgroup_mutex);
mutex is held during original allocation code below ...
> - if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> - u32 *tmp;
> -
> - /*
> - * If the architecture hasn't provided a sanitised value here,
> - * this may result in larger arrays than necessary. Resctrl will
> - * use a smaller system wide value based on the resources in
> - * use.
> - */
> - tmp = kcalloc(num_closid, sizeof(*tmp), GFP_KERNEL);
> - if (!tmp) {
> - err = -ENOMEM;
> - goto out_unlock;
> - }
> -
> - closid_num_dirty_rmid = tmp;
> - }
>
> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
> if (!rmid_ptrs) {
> - if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> - kfree(closid_num_dirty_rmid);
> - closid_num_dirty_rmid = NULL;
> - }
> err = -ENOMEM;
> goto out_unlock;
> }
> @@ -870,11 +848,6 @@ static void dom_data_exit(struct rdt_resource *r)
> if (!r->mon_capable)
> goto out_unlock;
>
> - if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> - kfree(closid_num_dirty_rmid);
> - closid_num_dirty_rmid = NULL;
> - }
> -
mutex is held in original free code ...
> kfree(rmid_ptrs);
> rmid_ptrs = NULL;
>
> @@ -957,9 +930,31 @@ int resctrl_l3_mon_resource_init(void)
> if (!r->mon_capable)
> return 0;
>
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + u32 num_closid = resctrl_arch_get_num_closid(r);
> + u32 *tmp;
> +
> + /*
> + * If the architecture hasn't provided a sanitised value here,
> + * this may result in larger arrays than necessary. Resctrl will
> + * use a smaller system wide value based on the resources in
> + * use.
> + */
> + tmp = kcalloc(num_closid, sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + closid_num_dirty_rmid = tmp;
> + }
mutex no longer held during allocation ... no mention in changelog why this is ok?
> +
> ret = dom_data_init(r);
> - if (ret)
> + if (ret) {
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + kfree(closid_num_dirty_rmid);
> + closid_num_dirty_rmid = NULL;
> + }
> return ret;
> + }
>
> if (resctrl_arch_is_evt_configurable(QOS_L3_MBM_TOTAL_EVENT_ID)) {
> mon_event_all[QOS_L3_MBM_TOTAL_EVENT_ID].configurable = true;
> @@ -984,5 +979,10 @@ void resctrl_l3_mon_resource_exit(void)
> {
> struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>
If you also use the "if (!r->mon_capable)" check as is done in original code
then resctrl_l3_mon_resource_exit() will be symmetrical with
resctrl_l3_mon_resource_init(). Since resctrl_l3_mon_resource_exit() is
_always_ called, whether monitoring is enabled or not, adding this check
will also make the code easier to follow and not require that reader knows
how closid_num_dirty_rmid[] is connected.
Another benefit of keeping this check is that it forces the function to
remain L3 resource specific (not just by name) and not seemingly become generic
later in series ("fs/resctrl: Move RMID initialization to first mount")
> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> + kfree(closid_num_dirty_rmid);
> + closid_num_dirty_rmid = NULL;
> + }
... now freed without mutex without mention in changelog?
> +
> dom_data_exit(r);
> }
Reinette