| Message ID | 20200526104642.9526-5-mike.leach@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update CoreSight infrastructure to select a default sink. | expand |
On 05/26/2020 11:46 AM, Mike Leach wrote: > Add default sink selection to the perf trace handling in the etm driver. > Uses the select default sink infrastructure to select a sink for the perf > session, if no other sink is specified. > > Signed-off-by: Mike Leach <mike.leach@linaro.org> This patch looks fine to me as such. But please see below for some discussion on the future support for other configurations. > --- > .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index 84f1dcb69827..1a3169e69bb1 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > sink = coresight_get_enabled_sink(true); > } > > - if (!sink) > - goto err; > - > mask = &event_data->mask; > > /* > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > continue; > } > > + /* > + * No sink provided - look for a default sink for one of the > + * devices. At present we only support topology where all CPUs > + * use the same sink [N:1], so only need to find one sink. The > + * coresight_build_path later will remove any CPU that does not > + * attach to the sink, or if we have not found a sink. > + */ > + if (!sink) > + sink = coresight_find_default_sink(csdev); > + While we are here, should we remove the "find enabled sink" if the csink is not specified via config. ? That step is problematic, as the user may not remember which sinks were enabled. Also, we can't hit that with perf tool as it prevents any invocation without sink (until this change). So may be this is a good time to get rid of that ? Also, we may need to do special handling for cases where there multiple sinks (ETRS) and the cpus in the event mask has different preferred sink. We can defer it for now as we don't claim to support such configurations yet. When we do, we could either : 1) Make sure the event is bound to a single CPU, in which case the sink remains the same for the event. OR 2) All the different "preferred" sinks (ETRs selected by the ETM) have the same capabilitiy. i.e, we can move around the "sink" specific buffers and use them where we end up using. We can defer all of this, until we get platforms which ned the support. Suzuki
Hi Suzuki, On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > On 05/26/2020 11:46 AM, Mike Leach wrote: > > Add default sink selection to the perf trace handling in the etm driver. > > Uses the select default sink infrastructure to select a sink for the perf > > session, if no other sink is specified. > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > This patch looks fine to me as such. But please see below for some > discussion on the future support for other configurations. > > > > --- > > .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > index 84f1dcb69827..1a3169e69bb1 100644 > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > sink = coresight_get_enabled_sink(true); > > } > > > > - if (!sink) > > - goto err; > > - > > mask = &event_data->mask; > > > > /* > > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > continue; > > } > > > > + /* > > + * No sink provided - look for a default sink for one of the > > + * devices. At present we only support topology where all CPUs > > + * use the same sink [N:1], so only need to find one sink. The > > + * coresight_build_path later will remove any CPU that does not > > + * attach to the sink, or if we have not found a sink. > > + */ > > + if (!sink) > > + sink = coresight_find_default_sink(csdev); > > + > > While we are here, should we remove the "find enabled sink" if the csink > is not specified via config. ? That step is problematic, as the user may > not remember which sinks were enabled. Also, we can't hit that with > perf tool as it prevents any invocation without sink (until this change). > > So may be this is a good time to get rid of that ? > You are correct - the 'sink = coresight_get_enabled_sink(true);' was dead code until this patch. However - if someone has set up their system using sysfs to enable sinks, then should we not respect that rather than assume they made a mistake? Thinking about N:M topologies mentioned below - one method of handling this is to enable relevant sinks and then let perf trace on any cores that might use them. > Also, we may need to do special handling for cases where there multiple > sinks (ETRS) and the cpus in the event mask has different preferred > sink. We can defer it for now as we don't claim to support such > configurations yet. Yes - the newer topologies will need some changes - beyond what we are handling here. However - especially for 1:1 - the best way may be to always use the default sink - as specifying multiple sinks on the perf command line may be problematical. > When we do, we could either : > > 1) Make sure the event is bound to a single CPU, in which case > the sink remains the same for the event. > > OR > > 2) All the different "preferred" sinks (ETRs selected by the ETM) have > the same capabilitiy. i.e, we can move around the "sink" specific > buffers and use them where we end up using. If here by "capabilities" we are talking about buffer vs system memory type sinks then I agree. We may need in future to limit the search algorithm to ensure that only the best system memory type sinks are found and eliminate cores attached to only buffer type sinks. > > We can defer all of this, until we get platforms which ned the support. > > Suzuki Thanks for the review. Mike
On 06/02/2020 02:12 PM, Mike Leach wrote: > Hi Suzuki, > > On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: >> >> On 05/26/2020 11:46 AM, Mike Leach wrote: >>> Add default sink selection to the perf trace handling in the etm driver. >>> Uses the select default sink infrastructure to select a sink for the perf >>> session, if no other sink is specified. >>> >>> Signed-off-by: Mike Leach <mike.leach@linaro.org> >> >> This patch looks fine to me as such. But please see below for some >> discussion on the future support for other configurations. >> >> >>> --- >>> .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- >>> 1 file changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c >>> index 84f1dcb69827..1a3169e69bb1 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c >>> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, >>> sink = coresight_get_enabled_sink(true); >>> } >>> >>> - if (!sink) >>> - goto err; >>> - >>> mask = &event_data->mask; >>> >>> /* >>> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, >>> continue; >>> } >>> >>> + /* >>> + * No sink provided - look for a default sink for one of the >>> + * devices. At present we only support topology where all CPUs >>> + * use the same sink [N:1], so only need to find one sink. The >>> + * coresight_build_path later will remove any CPU that does not >>> + * attach to the sink, or if we have not found a sink. >>> + */ >>> + if (!sink) >>> + sink = coresight_find_default_sink(csdev); >>> + >> >> While we are here, should we remove the "find enabled sink" if the csink >> is not specified via config. ? That step is problematic, as the user may >> not remember which sinks were enabled. Also, we can't hit that with >> perf tool as it prevents any invocation without sink (until this change). >> >> So may be this is a good time to get rid of that ? >> > > You are correct - the 'sink = coresight_get_enabled_sink(true);' was > dead code until this patch. > However - if someone has set up their system using sysfs to enable > sinks, then should we not respect that rather than assume they made a > mistake? If someone really wants to use a specific sink, then they could always specify it via the config attribute and it will be honoured. We need not carry along this non-intuitive hinting. > > Thinking about N:M topologies mentioned below - one method of handling > this is to enable relevant sinks and then let perf trace on any cores > that might use them. > >> Also, we may need to do special handling for cases where there multiple >> sinks (ETRS) and the cpus in the event mask has different preferred >> sink. We can defer it for now as we don't claim to support such >> configurations yet. > > Yes - the newer topologies will need some changes - beyond what we are > handling here. > However - especially for 1:1 - the best way may be to always use the > default sink - as specifying multiple sinks on the perf command line > may be problematical. > >> When we do, we could either : >> >> 1) Make sure the event is bound to a single CPU, in which case >> the sink remains the same for the event. >> >> OR >> >> 2) All the different "preferred" sinks (ETRs selected by the ETM) have >> the same capabilitiy. i.e, we can move around the "sink" specific >> buffers and use them where we end up using. > > If here by "capabilities" we are talking about buffer vs system memory > type sinks then I agree. We may need in future to limit the search Not necessarily. e.g, if we ever get two different types of system memory sinks, (e.g, a global ETR and a dedicate "new" sink for a cluster), we can't keep switching between the two sinks depending on how they use the buffers. (i.e, direct buffer vs double copy) Suzuki
On Tue, Jun 02, 2020 at 02:29:30PM +0100, Suzuki K Poulose wrote: > On 06/02/2020 02:12 PM, Mike Leach wrote: > > Hi Suzuki, > > > > On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > > > > On 05/26/2020 11:46 AM, Mike Leach wrote: > > > > Add default sink selection to the perf trace handling in the etm driver. > > > > Uses the select default sink infrastructure to select a sink for the perf > > > > session, if no other sink is specified. > > > > > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > > > > > This patch looks fine to me as such. But please see below for some > > > discussion on the future support for other configurations. > > > > > > > > > > --- > > > > .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > index 84f1dcb69827..1a3169e69bb1 100644 > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > > > sink = coresight_get_enabled_sink(true); > > > > } > > > > > > > > - if (!sink) > > > > - goto err; > > > > - > > > > mask = &event_data->mask; > > > > > > > > /* > > > > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > > > continue; > > > > } > > > > > > > > + /* > > > > + * No sink provided - look for a default sink for one of the > > > > + * devices. At present we only support topology where all CPUs > > > > + * use the same sink [N:1], so only need to find one sink. The > > > > + * coresight_build_path later will remove any CPU that does not > > > > + * attach to the sink, or if we have not found a sink. > > > > + */ > > > > + if (!sink) > > > > + sink = coresight_find_default_sink(csdev); > > > > + > > > > > > While we are here, should we remove the "find enabled sink" if the csink > > > is not specified via config. ? That step is problematic, as the user may > > > not remember which sinks were enabled. Also, we can't hit that with > > > perf tool as it prevents any invocation without sink (until this change). Old version of perf tools will take sinks selected on the perf command line and use the sysfs to communicate that to the kernel. Granted there may not be that many (if any), removing coresight_get_enabled_sink() will break those implementation. The real question is if keeping the functionatlity around so troublesome that it overweighs the drawbacks of removing it. > > > > > > So may be this is a good time to get rid of that ? > > > > > > > You are correct - the 'sink = coresight_get_enabled_sink(true);' was > > dead code until this patch. > > However - if someone has set up their system using sysfs to enable > > sinks, then should we not respect that rather than assume they made a > > mistake? > > If someone really wants to use a specific sink, then they could always > specify it via the config attribute and it will be honoured. We need not > carry along this non-intuitive hinting. > > > > > Thinking about N:M topologies mentioned below - one method of handling > > this is to enable relevant sinks and then let perf trace on any cores > > that might use them. > > > > > Also, we may need to do special handling for cases where there multiple > > > sinks (ETRS) and the cpus in the event mask has different preferred > > > sink. We can defer it for now as we don't claim to support such > > > configurations yet. > > > > Yes - the newer topologies will need some changes - beyond what we are > > handling here. > > However - especially for 1:1 - the best way may be to always use the > > default sink - as specifying multiple sinks on the perf command line > > may be problematical. > > > > > When we do, we could either : > > > > > > 1) Make sure the event is bound to a single CPU, in which case > > > the sink remains the same for the event. > > > > > > OR > > > > > > 2) All the different "preferred" sinks (ETRs selected by the ETM) have > > > the same capabilitiy. i.e, we can move around the "sink" specific > > > buffers and use them where we end up using. > > > > If here by "capabilities" we are talking about buffer vs system memory > > type sinks then I agree. We may need in future to limit the search > > Not necessarily. e.g, if we ever get two different types of system > memory sinks, (e.g, a global ETR and a dedicate "new" sink for a > cluster), we can't keep switching between the two sinks depending on how > they use the buffers. (i.e, direct buffer vs double copy) > > Suzuki
Hi, On Tue, 2 Jun 2020 at 17:59, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > On Tue, Jun 02, 2020 at 02:29:30PM +0100, Suzuki K Poulose wrote: > > On 06/02/2020 02:12 PM, Mike Leach wrote: > > > Hi Suzuki, > > > > > > On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote: > > > > > > > > On 05/26/2020 11:46 AM, Mike Leach wrote: > > > > > Add default sink selection to the perf trace handling in the etm driver. > > > > > Uses the select default sink infrastructure to select a sink for the perf > > > > > session, if no other sink is specified. > > > > > > > > > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > > > > > > > This patch looks fine to me as such. But please see below for some > > > > discussion on the future support for other configurations. > > > > > > > > > > > > > --- > > > > > .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- > > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > > index 84f1dcb69827..1a3169e69bb1 100644 > > > > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > > > > > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > > > > sink = coresight_get_enabled_sink(true); > > > > > } > > > > > > > > > > - if (!sink) > > > > > - goto err; > > > > > - > > > > > mask = &event_data->mask; > > > > > > > > > > /* > > > > > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > > > > > continue; > > > > > } > > > > > > > > > > + /* > > > > > + * No sink provided - look for a default sink for one of the > > > > > + * devices. At present we only support topology where all CPUs > > > > > + * use the same sink [N:1], so only need to find one sink. The > > > > > + * coresight_build_path later will remove any CPU that does not > > > > > + * attach to the sink, or if we have not found a sink. > > > > > + */ > > > > > + if (!sink) > > > > > + sink = coresight_find_default_sink(csdev); > > > > > + > > > > > > > > While we are here, should we remove the "find enabled sink" if the csink > > > > is not specified via config. ? That step is problematic, as the user may > > > > not remember which sinks were enabled. Also, we can't hit that with > > > > perf tool as it prevents any invocation without sink (until this change). > > Old version of perf tools will take sinks selected on the perf command line and > use the sysfs to communicate that to the kernel. Granted there may not be that > many (if any), removing coresight_get_enabled_sink() will break those > implementation. > > The real question is if keeping the functionatlity around so troublesome that it > overweighs the drawbacks of removing it. > > > > > > > > > So may be this is a good time to get rid of that ? > > > > > > > > > > You are correct - the 'sink = coresight_get_enabled_sink(true);' was > > > dead code until this patch. > > > However - if someone has set up their system using sysfs to enable > > > sinks, then should we not respect that rather than assume they made a > > > mistake? > > > > If someone really wants to use a specific sink, then they could always > > specify it via the config attribute and it will be honoured. We need not > > carry along this non-intuitive hinting. > > Problem is - as mentioned below - config can only specify one sink, so when we support 1:1 / N:M topology we need a way of specifying multiple sinks. This is one viable option - especially where we are using entire system configuration settings. As Mathieu points out - there is little harm in leaving this in - if we take it out now, we will probably have to replace it with something similar anyway. > > > > > > Thinking about N:M topologies mentioned below - one method of handling > > > this is to enable relevant sinks and then let perf trace on any cores > > > that might use them. > > > > > > > Also, we may need to do special handling for cases where there multiple > > > > sinks (ETRS) and the cpus in the event mask has different preferred > > > > sink. We can defer it for now as we don't claim to support such > > > > configurations yet. > > > > > > Yes - the newer topologies will need some changes - beyond what we are > > > handling here. > > > However - especially for 1:1 - the best way may be to always use the > > > default sink - as specifying multiple sinks on the perf command line > > > may be problematical. > > > > > > > When we do, we could either : > > > > > > > > 1) Make sure the event is bound to a single CPU, in which case > > > > the sink remains the same for the event. > > > > > > > > OR > > > > > > > > 2) All the different "preferred" sinks (ETRs selected by the ETM) have > > > > the same capabilitiy. i.e, we can move around the "sink" specific > > > > buffers and use them where we end up using. > > > > > > If here by "capabilities" we are talking about buffer vs system memory > > > type sinks then I agree. We may need in future to limit the search > > > > Not necessarily. e.g, if we ever get two different types of system > > memory sinks, (e.g, a global ETR and a dedicate "new" sink for a > > cluster), we can't keep switching between the two sinks depending on how > > they use the buffers. (i.e, direct buffer vs double copy) > > I would have thought it is an issue for the sink driver to sort this out on an individual basis. Multiple sinks I would think implies multiple perf buffers per intelPT, so each sink should have its own buffer, and hence deal with it in a sink specific way? Anyhow - as you say - something that can be deferred till we add the multi-sink support. Cheers Mike > > Suzuki -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
On Tue, May 26, 2020 at 11:46:41AM +0100, Mike Leach wrote: > Add default sink selection to the perf trace handling in the etm driver. > Uses the select default sink infrastructure to select a sink for the perf > session, if no other sink is specified. > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > --- > .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index 84f1dcb69827..1a3169e69bb1 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > sink = coresight_get_enabled_sink(true); > } > > - if (!sink) > - goto err; > - > mask = &event_data->mask; > > /* > @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > continue; > } > > + /* > + * No sink provided - look for a default sink for one of the > + * devices. At present we only support topology where all CPUs > + * use the same sink [N:1], so only need to find one sink. The > + * coresight_build_path later will remove any CPU that does not > + * attach to the sink, or if we have not found a sink. > + */ > + if (!sink) > + sink = coresight_find_default_sink(csdev); > + > /* > * Building a path doesn't enable it, it simply builds a > * list of devices from source to sink that can be > @@ -267,6 +274,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > *etm_event_cpu_path_ptr(event_data, cpu) = path; > } > > + /* no sink found for any CPU - cannot trace */ > + if (!sink) > + goto err; > + Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > /* If we don't have any CPUs ready for tracing, abort */ > cpu = cpumask_first(mask); > if (cpu >= nr_cpu_ids) > -- > 2.17.1 >
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 84f1dcb69827..1a3169e69bb1 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, sink = coresight_get_enabled_sink(true); } - if (!sink) - goto err; - mask = &event_data->mask; /* @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, continue; } + /* + * No sink provided - look for a default sink for one of the + * devices. At present we only support topology where all CPUs + * use the same sink [N:1], so only need to find one sink. The + * coresight_build_path later will remove any CPU that does not + * attach to the sink, or if we have not found a sink. + */ + if (!sink) + sink = coresight_find_default_sink(csdev); + /* * Building a path doesn't enable it, it simply builds a * list of devices from source to sink that can be @@ -267,6 +274,10 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, *etm_event_cpu_path_ptr(event_data, cpu) = path; } + /* no sink found for any CPU - cannot trace */ + if (!sink) + goto err; + /* If we don't have any CPUs ready for tracing, abort */ cpu = cpumask_first(mask); if (cpu >= nr_cpu_ids)
Add default sink selection to the perf trace handling in the etm driver. Uses the select default sink infrastructure to select a sink for the perf session, if no other sink is specified. Signed-off-by: Mike Leach <mike.leach@linaro.org> --- .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)