Hi,
On Thu, 7 May 2020 at 18:45, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Wed, 6 May 2020 at 16:55, Suzuki K Poulose suzuki.poulose@arm.com wrote:
On 05/06/2020 08:43 PM, Mathieu Poirier wrote:
On Wed, 6 May 2020 at 11:00, Mike Leach mike.leach@linaro.org wrote:
Hi Mathieu,
On Wed, 6 May 2020 at 17:49, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Wed, 6 May 2020 at 04:56, Mike Leach mike.leach@linaro.org wrote:
Hi Mathieu,
On Mon, 4 May 2020 at 21:31, Mathieu Poirier mathieu.poirier@linaro.org wrote: > > On Fri, May 01, 2020 at 03:02:22PM +0100, Mike Leach wrote: >> Adds a method to select a suitable sink connected to a given source. >> >> In cases where no sink is defined, the coresight_find_default_sink >> routine can search from a given source, through the child connections >> until a suitable sink is found. >> >> The suitability is defined in a sink_select_priority parameter on the >> CoreSight device. >> >> This allows for default sink to be discovered were none is specified >> (e.g. perf command line) >> >> Signed-off-by: Mike Leach mike.leach@linaro.org >> --- >> drivers/hwtracing/coresight/coresight-priv.h | 2 + >> drivers/hwtracing/coresight/coresight.c | 76 ++++++++++++++++++++ >> include/linux/coresight.h | 10 ++- >> 3 files changed, 87 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h >> index 890f9a5c97c6..856d8a29658c 100644 >> --- a/drivers/hwtracing/coresight/coresight-priv.h >> +++ b/drivers/hwtracing/coresight/coresight-priv.h >> @@ -150,6 +150,8 @@ int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data); >> struct coresight_device *coresight_get_sink(struct list_head *path); >> struct coresight_device *coresight_get_enabled_sink(bool reset); >> struct coresight_device *coresight_get_sink_by_id(u32 id); >> +struct coresight_device * >> +coresight_find_default_sink(struct coresight_device *csdev); >> struct list_head *coresight_build_path(struct coresight_device *csdev, >> struct coresight_device *sink); >> void coresight_release_path(struct list_head *path); >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c >> index c71553c09f8e..0564d26f5eb3 100644 >> --- a/drivers/hwtracing/coresight/coresight.c >> +++ b/drivers/hwtracing/coresight/coresight.c >> @@ -769,6 +769,82 @@ void coresight_release_path(struct list_head *path) >> path = NULL; >> } >> >> +/** >> + * coresight_find_sink - recursive function to walk trace connections from >> + * source to find a suitable default sink. >> + * >> + * @csdev: source / current device to check. >> + * @first_sink: the first lower priority sink found. >> + * >> + * Sinks are given priority weighting, the first HI priority sink will be >> + * used, if not found then the next highest priority sink will be recorded >> + * for possible later use. >> + * (Higher numbers indicate lower priorities, 0 not a sink or not to be >> + * selected as default) > > I think we should go for higher number == higher priority because 1) it is more > intuitive and 2) it allows us to grow with minimal changes. Right now ETB10, > TMC-ETF and TPIU are '0' and ETR is '1'. If new IP comes out we can assign them > higher numbers with little changes. >
Went for this as it matches priority scheme for linux kernel where 'nice' numbers have higher value for lower priority (only difference being we use 0 as a "don't use" value and never use -ve values) & I was thinking 1st priority is more important than 2nd priority etc.
The "nice" priorities is one of the least intuitive heuristic to ever come out of the kernel and I would like to avoid any design that follows the same pattern.
>> + * >> + * This will walk the connection path from a source (etm) till a suitable >> + * sink is encountered and return that sink to the original caller. >> + * >> + * return HI priority sink, or NULL if not found. >> + * Best lower priority sink returned in first_sink. >> + */ >> +static struct coresight_device * >> +coresight_find_sink(struct coresight_device *csdev, >> + struct coresight_device **first_sink) >> +{ >> + int i; >> + >> + /* return if highest priority sink */ >> + if (csdev->sink_select_priority == CORESIGHT_SINK_SELECT_PRIORITY_HI) >> + return csdev; >> + >> + /* record if lower priority sink */ >> + if (csdev->sink_select_priority > CORESIGHT_SINK_SELECT_PRIORITY_HI) { >> + if ((*first_sink == NULL) || >> + ((*first_sink)->sink_select_priority > >> + csdev->sink_select_priority)) { >> + *first_sink = csdev; >> + } >> + } >> + >> + /* >> + * Not a sink we want - recursively explore each port found on >> + * this element. >> + */ >> + for (i = 0; i < csdev->pdata->nr_outport; i++) { >> + struct coresight_device *child_dev, *sink; >> + >> + child_dev = csdev->pdata->conns[i].child_dev; >> + if (child_dev) { >> + sink = coresight_find_sink(child_dev, first_sink); >> + if (sink) >> + return sink; > > If @csdev is a replicator and a sink is found on port '0' components connected, > directly or not, to port '1' won't be searched.
Correct - by design - return the first found highest priority sink.
> Also, instead of contending > with a 'sink' and a 'first_sink' I would simply make coresight_find_sink() > return the highest priority sink found from any @csdev port. That should > simplify thing a little.
sink is used to early terminate the search if we find a sink that has the highest possible priority.
For now this will work but it won't when 1:1 topologies start coming around. I would like to find a solution where we can deal with N:1, 1:1 and 1:1 + global sink topologies.
I don't understand why you think this will not work with other topologies. It will work perfectly well with 1:1 as it walks from source to sink, so will hit the correct sink for the source.
1:1 will work but 1:1 + global sink won't if the topology is organised in such a way that the global (and common) sink is discovered first. I would like to believe a common sink won't be found in 1:1 topologies but that isn't guaranteed.
> >> + } >> + } >> + return NULL; >> +} >> + >> +/** >> + * coresight_find_default_sink: Find a sink suitable for use as a >> + * default sink. >> + * >> + * @csdev: starting source to find a connected sink. >> + * >> + * Walks connections graph looking for a suitable sink to enable for the >> + * supplied source. Uses sink_select_priority values. >> + * >> + * Used in cases where the CoreSight user (perf / sysfs) has not selected a >> + * sink. >> + */ >> +struct coresight_device * >> +coresight_find_default_sink(struct coresight_device *csdev) >> +{ >> + struct coresight_device *first_sink = NULL, *sink; >> + >> + sink = coresight_find_sink(csdev, &first_sink); >> + return sink ? sink : first_sink; >> +} >> + >> /** coresight_validate_source - make sure a source has the right credentials >> * @csdev: the device structure for a source. >> * @function: the function this was called from. >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >> index 193cc9dbf448..ff443761c8fc 100644 >> --- a/include/linux/coresight.h >> +++ b/include/linux/coresight.h >> @@ -161,10 +161,13 @@ struct coresight_connection { >> * @enable: 'true' if component is currently part of an active path. >> * @activated: 'true' only if a _sink_ has been activated. A sink can be >> * activated but not yet enabled. Enabling for a _sink_ >> - * appens when a source has been selected for that it. >> + * happens when a source has been selected for that it. > > This is obviously wrong, so much I can't even figure out what I meant. Please > remove the entire sentence in a cleanup patch that goes at the beginning of this > set. > >> * @ea: Device attribute for sink representation under PMU directory. >> * @ect_dev: Associated cross trigger device. Not part of the trace data >> * path or connections. >> + * @sink_select_priority: Used when the system is required to choose a default >> + * sink. Higher numbers indicate lower priority. Unused (0) if not >> + * a sink / or sink not supported as a default. >> */ >> struct coresight_device { >> struct coresight_platform_data *pdata; >> @@ -180,8 +183,13 @@ struct coresight_device { >> struct dev_ext_attribute *ea; >> /* cross trigger handling */ >> struct coresight_device *ect_dev; >> + /* mark priority when system choosing a default sink */ >> + int sink_select_priority; > > When I first looked at this set I didn't think there was a need for something > like this but after reflection we do since information about the kind of sink > (ETB10, TMC-ETF/ETR, TPIU) isn't available from the coresight_device. You can > also forget my comment about things not working for ETB10 and TPIU, it is > clearer now. > > I would simply call this "priority" and move it in the sink section, i.e just > under @activated. For extra credit please move @ea after @ops (also in a prequel > patch) since it is used for all coresight_device. >
could do this - or rename per suzukis suggestion.
>> }; >> >> +/* The highest priority sink - will terminate default sink search */ >> +#define CORESIGHT_SINK_SELECT_PRIORITY_HI 1 > > Why not calling this CORESIGHT_PRIORITY_ETR, so that when the next best thing > comes out we can have CORESIGHT_PRIORITY_NEXT_BEST_THING. Hopefully we won't > need to do that because everyone will move to 1:1 topologies but I know better. > I wanted to completely decouple the "Type" of sink from the selection priority. It is not important that it is an ETR now or maybe something else later. If something new comes along that is better then it is up to the sink drivers to allocate priorities accordingly. At present it is simple as a single driver covers the sinks of interest, but in future we may need to be more dynamic about allocating sinks.
When newETR comes out CORESIGHT_PRIORITY_NEWETR can be defined with a priority of '2'. With higher number == higher priority we wouldn't have to change any other driver, just assign the new priority in the probe() code of newETR.
Just like the search algorithm above, we may have to think further about what we want to do.
The search algorithm was predicated on a consensus I thought was reached in the previous discussions on this matter - do a simple find first ETR.
You are correct, we did have a consensus but what we agreed on clearly has some drawbacks. As with everyone else I am unhappy to revisit the topic but I'd rather do that now than having to live with something that doesn't scale.
I am happy to change this again if this is no longer agreed as the way forwards.
I think that in 1:1 and 1:1 + global sink topologies the sink closest to the ETM should be selected, regardless of priorities. Things get trickier with N:1 topologies and, given all the possible scenarios, I am wondering if using priorities at all is the way to go.
We could have per CPU sinks and global sinks.
Correct, and that is a topology that creates a lot of problems.
So if we find the closest "sink" from the ETM, we should be fine. Also we need not impose priority among the "sinks" that can route the trace to system memory. i.e, we could think of the sink "subtypes" as the implicit priority.
Given we want to use something that can stream the trace to memory on a sink (ETR, Future IPs) and not something with an internal memory (ETB/ETF/ETB10), could we simply use the coresight_dev_subtype_sink instead of defining priorities ? i.e: Add a new subtypefor SINKs that can push the trace to system memory.
enum coresight_dev_subtype_sink { CORESIGHT_DEV_SUBTYPE_SINK_NONE, CORESIGHT_DEV_SUBTYPE_SINK_PORT, CORESIGHT_DEV_SUBTYPE_SINK_BUFFER,
CORESIGHT_DEV_SUBTYPE_SINK_SYS_RAM,};
I had avoided adding a new sub-type as it implied a lot of rework of code beyond just the current problem. However, a more careful look has shown that CORESIGHT_DEV_SUBTYPE_SINK* are never actually used anywhere at present so this may be a realistic option.,
I like that part as it saves us from adding a new field to struct coresight_device.
Then the priority is implicit by higher subtype_sink value. With this and the closest sink (no of components to the sink) can be easily done.
I can't remember if we have considered the device tree before, but that would be really easy to do. We could introduce a new "default" property, only valid on sink devices, that would indicate what sink to
The first patchset was based on exactly this. It was felt this was not a good approach as ACPI support would be needed too.
use if one isn't explicitly given on the command line. If a sink isn't specified on the command line and a default sink wasn't specified in the DT, then we take the sink closest to the ETM.
Personally I do not prefer DT approach.
Same here.
Something like the below should work for all the cases ?
struct coresight_device * coresight_find_sink(struct coresight_device *dev, int *pdepth) { int i, depth; struct coresight_device *res = NULL;
/* We have reached sink at the given depth, end search */ if (dev->type == SINK) return dev; for (i = 0; i < dev->nr_outports; i ++) { struct coresight_device *tmp; int t = 0; if (!dev->conns[i].child_dev) continue; tmp = coresight_find_sink(dev->conns[i].child_dev, &t); if (!tmp) continue; /* First sink found, set initialize the result */ if (!res) { res = tmp; depth = t; continue; } /* * Update the result if : * a) if we have found a better sink type * b) Or something closer than the current one with * same capability */ if ((res->subtype.sink_subtype < tmp->subtype.sink_subtype) || (t < depth)) { depth = t; res = tmp; }In a 1:1 + global sink topology where the CPU sinks are ETB and the global sink is an ETR, if the global sink is discovered after a CPU sink, the global sink will be selected. In a N:1 topology, if a TPIU is located closest to the ETMs and discovered after an ETR, the TPIU will be selected.I think
The priority schema used in the current patch set deliberately discounts TPIU as a selectable sink. This must be a feature of any automated scheme as selecting a TPIU (or any off-chip sink) fior purely self hosted trace can never be valid.
If we consider the CS-BSA document, using ETB as a 1:1 sink would not be recommended - it requires that 1:1 topologies use trace capture to system memory. An intervening ETF between ETM and ETR is permitted. Here we can discount a 1:1 schema witn ETM:ETB, and reasonably assume either ETM:<ETR or similar> or <ETM>:<ETF>:<ETR or similar>
The problem is that our search criteria are changing based on the mode we're in. 1:1 we want the shortest distance from source while N:1 a type of sink is preferred. Finding the kind of topology we have on a system will allow us to select the right search heuristic. One way to do that is to keep track of the amount of sinks registered on a system as they get registered. If nr_sinks >= NR_CPUs, we have a 1:1 or a 1:1 + global sink. Otherwise we're back to the good old N:1.
not necessarily true - a 4 core 2 cluster config, with ETF per cluster + ETR + TPIU actually has 4 sinks and 4 cores, but is N:1. (juno-r1/2 gets close to this with a 6:4 CPU:sink ratio).
I don't think we do need to change the search criteria - just get the prioritisation correct. Use priority + distance to pick, prioritize system memory devices (ETR etc) over local buffer devices (ETF/ETB). This should be sufficient for current and envisaged (by CS-BSA) future systems.
Regards
Mike
What do you guys think?
} /* Adjust the depth to accommodate the current dev */ if (res && pdepth) *pdepth = depth + 1; return res;}
}
struct coresight_device * coresight_find_default_sink(source) { return coresight_find_sink(source, NULL); }
Thoughts ?
Suzuki
Let me know what you think...
Thanks, Mathieu
Regards
Mike
> For you next revision, please rebase on coresight-next, I couldn't apply this > one properly. >
Sorry - was using Linux-5.7-rc3 main branch. Hadn;t realised there had been sufficient diversion that this would not also work on your coresight branch.
Mike
> Thanks, > Mathieu > > >> + >> /* >> * coresight_dev_list - Mapping for devices to "name" index for device >> * names. >> -- >> 2.17.1 >>
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK