Hi Mike,
I am from the Android toolchain team. We are interested in using the ETM strobe feature in Android kernels. We noticed your patch "coresight: etmv4: Adds initial complex config with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html. We tested it on Pixel devices with Qualcomm chips. It works pretty well. And I have a few questions:
1. The param setting of window and period seems doesn't take effect. Not sure if it is a bug or something I misunderstand.
The only place I found moving feat->params to feat->regs is in cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params. So if I write a value to /sys/.../window, it goes to feat->params, but can't change value in feat->regs. I tried a work-around by changing cs_cfg_param_store(): * static ssize_t cs_cfg_param_store(struct device *dev, {*
* ... spin_lock(param->feat->dev_spinlock);* * if (!kstrtoull(buf, 0, &val)) {*
* param->current_value = val; for (i = 0; i < feat->nr_regs; i++) { reg_desc = &feat_desc->regs[i];* * flags = reg_desc->flags;* * reg = &feat->regs[i];* * if (flags & CS_CFG_REG_VAL_PARAM) {* * if (param == &feat->params[reg_desc->value.val32]) {*
* coresight_cfg_init_reg_param(reg, flags, param); }* * }* * }* * }* * spin_unlock(param->feat->dev_spinlock);* * }*
2. Is my understanding below correct? If I set the window param to 1000 and the period param to 10000, then etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles? And since the strobe window/period counters are saved when disabling etm, the counting should continue from the saved value the next time I enable etm. And I should still get etm data even if the traced thread is scheduled off cpu before reaching 10^7 cpu cycles each time.
3. I want to +1 Mathieu's suggestion:
*Perf sessions need to be able to choose which feature they want to use rather than defaulting to a system wide configuration.*
We use perf sessions to trace ETM data in Android. It's better if we can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
4. The last update on the patch is Jun 24. And it seems the patch hasn't been landed in any branch yet. So what's the future plan?
We need to wait until it lands to some upstream branches to cherry pick it to Android kernels. So I want to know when it will be ready.
Thanks,
Yabin
ping?
On Thu, Sep 3, 2020 at 12:25 PM Yabin Cui yabinc@google.com wrote:
Hi Mike,
I am from the Android toolchain team. We are interested in using the ETM strobe feature in Android kernels. We noticed your patch "coresight: etmv4: Adds initial complex config with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html. We tested it on Pixel devices with Qualcomm chips. It works pretty well. And I have a few questions:
- The param setting of window and period seems doesn't take effect. Not
sure if it is a bug or something I misunderstand.
The only place I found moving feat->params to feat->regs is in cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params. So if I write a value to /sys/.../window, it goes to feat->params, but can't change value in feat->regs. I tried a work-around by changing cs_cfg_param_store():
static ssize_t cs_cfg_param_store(struct device *dev, {*... spin_lock(param->feat->dev_spinlock);*if (!kstrtoull(buf, 0, &val)) {*param->current_value = val; for (i = 0;i < feat->nr_regs; i++) { reg_desc = &feat_desc->regs[i];*
flags = reg_desc->flags;*reg = &feat->regs[i];*if (flags & CS_CFG_REG_VAL_PARAM) {*if (param ==&feat->params[reg_desc->value.val32]) {*
coresight_cfg_init_reg_param(reg, flags, param); }*
}*}*}*spin_unlock(param->feat->dev_spinlock);*- }*
- Is my understanding below correct?
If I set the window param to 1000 and the period param to 10000, then etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles? And since the strobe window/period counters are saved when disabling etm, the counting should continue from the saved value the next time I enable etm. And I should still get etm data even if the traced thread is scheduled off cpu before reaching 10^7 cpu cycles each time.
- I want to +1 Mathieu's suggestion:
*Perf sessions need to be able to choose which feature they want to use rather than defaulting to a system wide configuration.*
We use perf sessions to trace ETM data in Android. It's better if we can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
- The last update on the patch is Jun 24. And it seems the patch hasn't been landed in any branch yet. So what's the future plan?
We need to wait until it lands to some upstream branches to cherry pick it to Android kernels. So I want to know when it will be ready.
Thanks,
Yabin
Hi Yabin,
Sorry for the delay - your e-mail arrived while I was on holiday & I have been playing catch up since getting back.
On Mon, 14 Sep 2020 at 18:16, Yabin Cui yabinc@google.com wrote:
ping?
On Thu, Sep 3, 2020 at 12:25 PM Yabin Cui yabinc@google.com wrote:
Hi Mike,
I am from the Android toolchain team. We are interested in using the ETM strobe feature in Android kernels. We noticed your patch "coresight: etmv4: Adds initial complex config with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html. We tested it on Pixel devices with Qualcomm chips. It works pretty well. And I have a few questions:
The param setting of window and period seems doesn't take effect. Not sure if it is a bug or something I misunderstand.
The only place I found moving feat->params to feat->regs is in cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params. So if I write a value to /sys/.../window, it goes to feat->params, but can't change value in feat->regs. I tried a work-around by changing cs_cfg_param_store(): static ssize_t cs_cfg_param_store(struct device *dev, { ... spin_lock(param->feat->dev_spinlock); if (!kstrtoull(buf, 0, &val)) { param->current_value = val; for (i = 0; i < feat->nr_regs; i++) { reg_desc = &feat_desc->regs[i]; flags = reg_desc->flags; reg = &feat->regs[i]; if (flags & CS_CFG_REG_VAL_PARAM) { if (param == &feat->params[reg_desc->value.val32]) { coresight_cfg_init_reg_param(reg, flags, param); } } } } spin_unlock(param->feat->dev_spinlock); }
OK - I may have fixed this in the new set - please try this out:- https://lists.linaro.org/pipermail/coresight/2020-August/004731.html
- Is my understanding below correct?
If I set the window param to 1000 and the period param to 10000, then etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles? And since the strobe window/period counters are saved when disabling etm, the counting should continue from the saved value the next time I enable etm. And I should still get etm data even if the traced thread is scheduled off cpu before reaching 10^7 cpu cycles each time.
That is essentially it. I was not the original author of the strobing routine, so cannot remember if it is the first window or the last window that is traced, but there are period x windows, with trace captured for a single window.
- I want to +1 Mathieu's suggestion:
Perf sessions need to be able to choose which feature they want to use rather than defaulting to a system wide configuration.
Again look at the new set - perf can select a configuration - which can include strobing. The new set has an example of this in operation.
We use perf sessions to trace ETM data in Android. It's better if we can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
- The last update on the patch is Jun 24. And it seems the patch hasn't been landed in any branch yet. So what's the future plan?
The patch set is to allow a generic method to configure more complex coresight programming settings, and make these easily selectable - from perf and any other client or sysfs. As such this is in RFC at the moment as we seek consensus on a way forwards. Once that is acheived then it will be pushed upstream.
We need to wait until it lands to some upstream branches to cherry pick it to Android kernels. So I want to know when it will be ready.
Thanks,
Yabin
Regards
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Mike,
Thanks for the reply! I tried the patch set on hikey960 board. It works well. And I can change strobe parameters in sysfs. I have two feedbacks: 1. There is an unbalanced lock in cscfg_load_config_sets() in coresight-syscfg.c. If cscfg_load_feat() or cscfg_load_config() returns an error, the function returns without mutex_unlock. 2. I think it's better to show more info of preset configs in cscfg_device in sysfs. It helps userspace tools know which preset to select.
Thanks, Yabin
On Tue, Sep 15, 2020 at 1:49 AM Mike Leach mike.leach@linaro.org wrote:
Hi Yabin,
Sorry for the delay - your e-mail arrived while I was on holiday & I have been playing catch up since getting back.
On Mon, 14 Sep 2020 at 18:16, Yabin Cui yabinc@google.com wrote:
ping?
On Thu, Sep 3, 2020 at 12:25 PM Yabin Cui yabinc@google.com wrote:
Hi Mike,
I am from the Android toolchain team. We are interested in using the
ETM strobe feature in Android kernels.
We noticed your patch "coresight: etmv4: Adds initial complex config
with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html.
We tested it on Pixel devices with Qualcomm chips. It works pretty well. And I have a few questions:
- The param setting of window and period seems doesn't take effect.
Not sure if it is a bug or something I misunderstand.
The only place I found moving feat->params to feat->regs is in
cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params.
So if I write a value to /sys/.../window, it goes to feat->params,
but can't change value in feat->regs.
I tried a work-around by changing cs_cfg_param_store(): static ssize_t cs_cfg_param_store(struct device *dev, { ... spin_lock(param->feat->dev_spinlock); if (!kstrtoull(buf, 0, &val)) { param->current_value = val; for (i = 0; i < feat->nr_regs; i++) { reg_desc = &feat_desc->regs[i]; flags = reg_desc->flags; reg = &feat->regs[i]; if (flags & CS_CFG_REG_VAL_PARAM) { if (param ==
&feat->params[reg_desc->value.val32]) {
coresight_cfg_init_reg_param(reg, flags, param);
} } } } spin_unlock(param->feat->dev_spinlock); }OK - I may have fixed this in the new set - please try this out:- https://lists.linaro.org/pipermail/coresight/2020-August/004731.html
- Is my understanding below correct?
If I set the window param to 1000 and the period param to 10000, then
etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles?
And since the strobe window/period counters are saved when disabling
etm, the counting should continue from the saved value the next time I enable etm. And I should still
get etm data even if the traced thread is scheduled off cpu before
reaching 10^7 cpu cycles each time.
That is essentially it. I was not the original author of the strobing routine, so cannot remember if it is the first window or the last window that is traced, but there are period x windows, with trace captured for a single window.
- I want to +1 Mathieu's suggestion:
Perf sessions need to be able to choose which feature they want to use
rather
than defaulting to a system wide configuration.
Again look at the new set - perf can select a configuration - which can include strobing. The new set has an example of this in operation.
We use perf sessions to trace ETM data in Android. It's better if we
can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
- The last update on the patch is Jun 24. And it seems the patch
hasn't been landed in any branch yet. So what's the future plan?
The patch set is to allow a generic method to configure more complex coresight programming settings, and make these easily selectable - from perf and any other client or sysfs. As such this is in RFC at the moment as we seek consensus on a way forwards. Once that is acheived then it will be pushed upstream.
We need to wait until it lands to some upstream branches to cherry pick
it to Android kernels. So I want to know when it will be ready.
Thanks,
Yabin
Regards
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
Hi Yabin,
On Thu, 1 Oct 2020 at 18:28, Yabin Cui yabinc@google.com wrote:
Hi Mike,
Thanks for the reply! I tried the patch set on hikey960 board. It works well. And I can change strobe parameters in sysfs.
Thanks for looking at this.
I have two feedbacks:
- There is an unbalanced lock in cscfg_load_config_sets() in coresight-syscfg.c. If cscfg_load_feat() or cscfg_load_config() returns an error, the function returns without mutex_unlock.
Good spot - I'll fix in the next release.
- I think it's better to show more info of preset configs in cscfg_device in sysfs. It helps userspace tools know which preset to select.
Agreed. The RFC is a minimum set to try to get some form of ETM strobing upstreamed for autofdo usage. I have a follow up patch set that represents the configurations and features in configfs - following Mathieu's request in RFC v1 to prefer that over sysfs.
At present this looks like:-
linaro@linaro-developer:/config$ ls coresight-syscfg stp-policy linaro@linaro-developer:/config$ cd coresight-syscfg/ linaro@linaro-developer:/config/coresight-syscfg$ ls config_list configurations feature_list linaro@linaro-developer:/config/coresight-syscfg$ cd configurations/ linaro@linaro-developer:/config/coresight-syscfg/configurations$ ls autofdo linaro@linaro-developer:/config/coresight-syscfg/configurations$ ls autofdo/ description nr_presets preset_idx preset_values uses linaro@linaro-developer:/config/coresight-syscfg/configurations$ cat autofdo/preset_values Preset[1], 2 parameters strobing.window = 0x2710 strobing.period = 0x1388 linaro@linaro-developer:/config/coresight-syscfg/configurations$ cat autofdo/uses uses 1 features:- strobing
So the configuration (and features) can be examined under configfs. It may be that setting of individual values for device features could also move to configfs from their current location in sysfs.
I am trying to split the functionality into a series of patchsets so as not to overwhelm reviewers.
Regards
Mike
Thanks, Yabin
On Tue, Sep 15, 2020 at 1:49 AM Mike Leach mike.leach@linaro.org wrote:
Hi Yabin,
Sorry for the delay - your e-mail arrived while I was on holiday & I have been playing catch up since getting back.
On Mon, 14 Sep 2020 at 18:16, Yabin Cui yabinc@google.com wrote:
ping?
On Thu, Sep 3, 2020 at 12:25 PM Yabin Cui yabinc@google.com wrote:
Hi Mike,
I am from the Android toolchain team. We are interested in using the ETM strobe feature in Android kernels. We noticed your patch "coresight: etmv4: Adds initial complex config with ETM4 strobe feature." in https://lists.linaro.org/pipermail/coresight/2020-June/004097.html. We tested it on Pixel devices with Qualcomm chips. It works pretty well. And I have a few questions:
The param setting of window and period seems doesn't take effect. Not sure if it is a bug or something I misunderstand.
The only place I found moving feat->params to feat->regs is in cs_cfg_reset_feat(). But that function will also reset feat->params to default values in feat_desc->params. So if I write a value to /sys/.../window, it goes to feat->params, but can't change value in feat->regs. I tried a work-around by changing cs_cfg_param_store(): static ssize_t cs_cfg_param_store(struct device *dev, { ... spin_lock(param->feat->dev_spinlock); if (!kstrtoull(buf, 0, &val)) { param->current_value = val; for (i = 0; i < feat->nr_regs; i++) { reg_desc = &feat_desc->regs[i]; flags = reg_desc->flags; reg = &feat->regs[i]; if (flags & CS_CFG_REG_VAL_PARAM) { if (param == &feat->params[reg_desc->value.val32]) { coresight_cfg_init_reg_param(reg, flags, param); } } } } spin_unlock(param->feat->dev_spinlock); }
OK - I may have fixed this in the new set - please try this out:- https://lists.linaro.org/pipermail/coresight/2020-August/004731.html
- Is my understanding below correct?
If I set the window param to 1000 and the period param to 10000, then etm will trace the last 1000 cpu cycles of every 10^7 cpu cycles? And since the strobe window/period counters are saved when disabling etm, the counting should continue from the saved value the next time I enable etm. And I should still get etm data even if the traced thread is scheduled off cpu before reaching 10^7 cpu cycles each time.
That is essentially it. I was not the original author of the strobing routine, so cannot remember if it is the first window or the last window that is traced, but there are period x windows, with trace captured for a single window.
- I want to +1 Mathieu's suggestion:
Perf sessions need to be able to choose which feature they want to use rather than defaulting to a system wide configuration.
Again look at the new set - perf can select a configuration - which can include strobing. The new set has an example of this in operation.
We use perf sessions to trace ETM data in Android. It's better if we can configure ETM strobe params in the perf interface (like from perf_event_attr or ioctl). And It's better for each perf session to count separately.
- The last update on the patch is Jun 24. And it seems the patch hasn't been landed in any branch yet. So what's the future plan?
The patch set is to allow a generic method to configure more complex coresight programming settings, and make these easily selectable - from perf and any other client or sysfs. As such this is in RFC at the moment as we seek consensus on a way forwards. Once that is acheived then it will be pushed upstream.
We need to wait until it lands to some upstream branches to cherry pick it to Android kernels. So I want to know when it will be ready.
Thanks,
Yabin
Regards
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
-
Mike Leach -
Yabin Cui