| Message ID | 20241202092419.11777-4-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Coresight TMC-ETR some bugfixes and cleanups | expand |
Hi Yicong On 02/12/2024 09:24, Yicong Yang wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > When trying to run perf and sysfs mode simultaneously, the WARN_ON() > in tmc_etr_enable_hw() is triggered sometimes: > > WARNING: CPU: 42 PID: 3911571 at drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] > [..snip..] > Call trace: > tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P) > tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L) > tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] > coresight_enable_path+0x1c8/0x218 [coresight] > coresight_enable_sysfs+0xa4/0x228 [coresight] > enable_source_store+0x58/0xa8 [coresight] > dev_attr_store+0x20/0x40 > sysfs_kf_write+0x4c/0x68 > kernfs_fop_write_iter+0x120/0x1b8 > vfs_write+0x2c8/0x388 > ksys_write+0x74/0x108 > __arm64_sys_write+0x24/0x38 > el0_svc_common.constprop.0+0x64/0x148 > do_el0_svc+0x24/0x38 > el0_svc+0x3c/0x130 > el0t_64_sync_handler+0xc8/0xd0 > el0t_64_sync+0x1ac/0x1b0 > ---[ end trace 0000000000000000 ]--- > > Since the enablement of sysfs mode is separeted into two critical regions, > one for sysfs buffer allocation and another for hardware enablement, it's > possible to race with the perf mode. Fix this by double check whether > the perf mode's been used before enabling the hardware in sysfs mode. Thanks for the fix. Some minor comments below. It needs a Fixes tag. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > .../hwtracing/coresight/coresight-tmc-etr.c | 30 +++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c > index ad83714ca4dc..d382d95da5ff 100644 > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c > @@ -1230,6 +1230,36 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) > > spin_lock_irqsave(&drvdata->spinlock, flags); > > + /* > + * Since the sysfs buffer allocation and the hardware enablement is not > + * in the same critical region, it's possible to race with the perf > + * mode: > + * [sysfs mode] [perf mode] > + * tmc_etr_get_sysfs_buffer() > + * spin_lock(&drvdata->spinlock) > + * [sysfs buffer allocation] > + * spin_unlock(&drvdata->spinlock) > + * spin_lock(&drvdata->spinlock) > + * tmc_etr_enable_hw() > + * drvdata->etr_buf = etr_perf->etr_buf > + * spin_unlock(&drvdata->spinlock) > + * spin_lock(&drvdata->spinlock) > + * tmc_etr_enable_hw() > + * WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at > + * the perf side > + * spin_unlock(&drvdata->spinlock) > + * > + * So check here before continue. > + */ > + if (coresight_get_mode(csdev) == CS_MODE_PERF) { > + drvdata->sysfs_buf = NULL; > + spin_unlock_irqrestore(&drvdata->spinlock, flags); > + > + /* Free allocated memory out side of the spinlock */ > + tmc_etr_free_sysfs_buf(sysfs_buf); > + return -EBUSY; > + } With this in place, I think we should remove the check for CS_MODE_PERF in get_etr_sysfs_buf() to avoid confusion (which I believe opened up this race) Suzuki > + > /* > * In sysFS mode we can have multiple writers per sink. Since this > * sink is already enabled no memory is needed and the HW need not be
On 2024/12/2 17:46, Suzuki K Poulose wrote: > Hi Yicong > > On 02/12/2024 09:24, Yicong Yang wrote: >> From: Yicong Yang <yangyicong@hisilicon.com> >> >> When trying to run perf and sysfs mode simultaneously, the WARN_ON() >> in tmc_etr_enable_hw() is triggered sometimes: >> >> WARNING: CPU: 42 PID: 3911571 at drivers/hwtracing/coresight/coresight-tmc-etr.c:1060 tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] >> [..snip..] >> Call trace: >> tmc_etr_enable_hw+0xc0/0xd8 [coresight_tmc] (P) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] (L) >> tmc_enable_etr_sink+0x11c/0x250 [coresight_tmc] >> coresight_enable_path+0x1c8/0x218 [coresight] >> coresight_enable_sysfs+0xa4/0x228 [coresight] >> enable_source_store+0x58/0xa8 [coresight] >> dev_attr_store+0x20/0x40 >> sysfs_kf_write+0x4c/0x68 >> kernfs_fop_write_iter+0x120/0x1b8 >> vfs_write+0x2c8/0x388 >> ksys_write+0x74/0x108 >> __arm64_sys_write+0x24/0x38 >> el0_svc_common.constprop.0+0x64/0x148 >> do_el0_svc+0x24/0x38 >> el0_svc+0x3c/0x130 >> el0t_64_sync_handler+0xc8/0xd0 >> el0t_64_sync+0x1ac/0x1b0 >> ---[ end trace 0000000000000000 ]--- >> >> Since the enablement of sysfs mode is separeted into two critical regions, >> one for sysfs buffer allocation and another for hardware enablement, it's >> possible to race with the perf mode. Fix this by double check whether >> the perf mode's been used before enabling the hardware in sysfs mode. > > Thanks for the fix. Some minor comments below. > > It needs a Fixes tag. > >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> .../hwtracing/coresight/coresight-tmc-etr.c | 30 +++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> index ad83714ca4dc..d382d95da5ff 100644 >> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c >> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c >> @@ -1230,6 +1230,36 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) >> spin_lock_irqsave(&drvdata->spinlock, flags); >> + /* >> + * Since the sysfs buffer allocation and the hardware enablement is not >> + * in the same critical region, it's possible to race with the perf >> + * mode: >> + * [sysfs mode] [perf mode] >> + * tmc_etr_get_sysfs_buffer() >> + * spin_lock(&drvdata->spinlock) >> + * [sysfs buffer allocation] >> + * spin_unlock(&drvdata->spinlock) >> + * spin_lock(&drvdata->spinlock) >> + * tmc_etr_enable_hw() >> + * drvdata->etr_buf = etr_perf->etr_buf >> + * spin_unlock(&drvdata->spinlock) >> + * spin_lock(&drvdata->spinlock) >> + * tmc_etr_enable_hw() >> + * WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at >> + * the perf side >> + * spin_unlock(&drvdata->spinlock) >> + * >> + * So check here before continue. >> + */ >> + if (coresight_get_mode(csdev) == CS_MODE_PERF) { >> + drvdata->sysfs_buf = NULL; >> + spin_unlock_irqrestore(&drvdata->spinlock, flags); >> + >> + /* Free allocated memory out side of the spinlock */ >> + tmc_etr_free_sysfs_buf(sysfs_buf); >> + return -EBUSY; >> + } > > With this in place, I think we should remove the check for CS_MODE_PERF in get_etr_sysfs_buf() to avoid confusion (which I believe opened up this race) > ok, considering that maybe we can optimize it a bit more in the tmc_etr_get_sysfs_buffer(). Check whether the perf mode's already running before we actually allocate the buffer. Then we can save the time of allocating/freeing the sysfs buffer if race with the perf mode. Thanks.
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index ad83714ca4dc..d382d95da5ff 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1230,6 +1230,36 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); + /* + * Since the sysfs buffer allocation and the hardware enablement is not + * in the same critical region, it's possible to race with the perf + * mode: + * [sysfs mode] [perf mode] + * tmc_etr_get_sysfs_buffer() + * spin_lock(&drvdata->spinlock) + * [sysfs buffer allocation] + * spin_unlock(&drvdata->spinlock) + * spin_lock(&drvdata->spinlock) + * tmc_etr_enable_hw() + * drvdata->etr_buf = etr_perf->etr_buf + * spin_unlock(&drvdata->spinlock) + * spin_lock(&drvdata->spinlock) + * tmc_etr_enable_hw() + * WARN_ON(drvdata->etr_buf) // WARN sicne etr_buf initialized at + * the perf side + * spin_unlock(&drvdata->spinlock) + * + * So check here before continue. + */ + if (coresight_get_mode(csdev) == CS_MODE_PERF) { + drvdata->sysfs_buf = NULL; + spin_unlock_irqrestore(&drvdata->spinlock, flags); + + /* Free allocated memory out side of the spinlock */ + tmc_etr_free_sysfs_buf(sysfs_buf); + return -EBUSY; + } + /* * In sysFS mode we can have multiple writers per sink. Since this * sink is already enabled no memory is needed and the HW need not be