Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 18 Apr 2024 16:31:14 +0100 | From | Dave Martin <> | Subject | Re: [PATCH v1 18/31] x86/resctrl: Allow resctrl_arch_mon_event_config_write() to return an error |
| |
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
| ![\](/images/icornerr.gif) |