Re: [PATCH v1 18/31] x86/resctrl: Allow resctrl_arch_mon_event_config_write() to return an error

[Dave Martin]

On Wed, Apr 17, 2024 at 10:19:31PM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/17/2024 7:42 AM, Dave Martin wrote:
> > Hi Rainette,
> > 
> > On Thu, Apr 11, 2024 at 10:39:37AM -0700, Reinette Chatre wrote:
> >> Hi Dave,
> >>
> >> On 4/11/2024 7:17 AM, Dave Martin wrote:
> >>> On Mon, Apr 08, 2024 at 08:23:36PM -0700, Reinette Chatre wrote:
> >>>> Hi James,
> >>>>
> >>>> On 3/21/2024 9:50 AM, James Morse wrote:
> >>>>> resctrl_arch_mon_event_config_write() writes a bitmap of events provided
> >>>>> by user-space into the configuration register for the monitors.
> >>>>>
> >>>>> This assumes that all architectures support all the features each bit
> >>>>> corresponds to.
> >>>>>
> >>>>> MPAM can filter monitors based on read, write, or both, but there are
> >>>>> many more options in the existing bitmap. To allow this interface to
> >>>>> work for machines with MPAM, allow the architecture helper to return
> >>>>> an error if an incompatible bitmap is set.
> >>>>>
> >>>>> When valid values are provided, there is no change in behaviour. If
> >>>>> an invalid value is provided, currently it is silently ignored, but
> >>>>> last_cmd_status is updated. After this change, the parser will stop
> >>>>> at the first invalid value and return an error to user-space. This
> >>>>> matches the way changes to the schemata file are made.
> >>>>>
> >>>>
> >>>> Is this needed? With move of mbm_cfg_mask to rdt_resource I expect
> >>>> MPAM would use it to set what the valid values are. With that done,
> >>>> when user space provides a value, mon_config_write() compares user
> >>>> provided value against mbm_cfg_mask and will already return early
> >>>> (before attempting to write to hardware) with error
> >>>> if value is not supported. This seems to accomplish the goal of this
> >>>> patch?
> >>>
> >>> This sounds plausible.
> >>>
> >>> In a recent snapshot of James' MPAM code, it looks like we could be
> >>> initialising rdt_resource::mbm_cfg_mask when setting up the rdt_resource
> >>> struct for resctrl, though in fact this information is captured
> >>> differently right now.  I'm sure why (though James may have a
> >>> reason). [1]
> >>>
> >>> I don't see an obvious reason though why we couldn't set mbm_cfg_mask
> >>> and detect bad config values globally in mon_config_write(), the same as
> >>> for the existing AMD BMEC case.
> >>>
> >>> Nothing in the MPAM architecture stops hardware vendors from randomly
> >>> implementing different capabilities in different components of the
> >>> system, but provided that we only expose the globally supported subset
> >>> of event filtering capabilities to resctrl this approach looks workable.
> >>> This consistent with the James' MPAM code deals with other feature
> >>> mismatches across the system today.
> >>>
> >>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_resctrl.c?h=mpam/snapshot/v6.7-rc2#n730
> >>
> >> My response was based on what I understood from the goal of this change
> >> as described by the changelog. The patch does not appear to match with
> >> the goals stated in changelog.
> >>
> >> As I understand the patch it aims to detect when there is an invalid
> >> event id. It is not possible for this scenario to occur because this code
> >> is always called with a valid event id.
> >>
> >> Reinette
> > 
> > I guess this will need discussion with James.  FWIW, my impression was
> > that the real goal of this patch was to allow a bad event config to be
> > detected at cross-call time and reported asynchronously.  Changes
> > elsewhere look to be there just to make error reporting consistent for
> > other existing paths too.
> 
> How do you interpret "bad event config"?
> 
> As I understand it, this patch only sets an error in one scenario:
> 
> 	 	index = mon_event_config_index_get(mon_info->evtid);
> 	 	if (index == INVALID_CONFIG_INDEX) {
>  			pr_warn_once("Invalid event id %d\n", mon_info->evtid);
> 			mon_info->err = -EINVAL;
> 	 		return;
>  		}
> 
> When will mon_info->evtid be anything but QOS_L3_MBM_TOTAL_EVENT_ID or
> QOS_L3_MBM_LOCAL_EVENT_ID? 
> 
> Reinette

I don't know; my reading of this was that since there was a pr_warn()
already, and since James was adding the capability to return an error,
he figured that a suitable error ought to be returned in this case.

But the real reason for the error return mechanism seems to be
resctrl_arch_mon_event_config_write() in the MPAM code, here:

https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=mpam/snapshot/v6.7-rc2&id=db0ac51f60675b6c4a54ccd24fa7198ec321c56d

I agree though that if we set mbm_cfg_mask in the rdt_resource at probe
time, the code in mon_config_write() ought to catch such cases cleanly
before making the cross-call.  So maybe the new mechanism isn't needed.

I think I need to discuss this with James, to figure out if there's any
reason why that wouldn't work.

Cheers
---Dave