Re: [PATCH v2 3/3] coresight: tmc: Decouple the perf buffer allocation from sysfs mode

[hejunhao]

On 2025/7/3 1:08, Leo Yan wrote:
> On Fri, Jun 20, 2025 at 03:54:12PM +0800, Junhao He wrote:
>
> [..]
>
> > @@ -1341,33 +1339,24 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
> >   	unsigned long size;
> >   
> >   	node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu);
> > -	/*
> > -	 * Try to match the perf ring buffer size if it is larger
> > -	 * than the size requested via sysfs.
> > -	 */
> > -	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
> > -		etr_buf = tmc_alloc_etr_buf(drvdata, ((ssize_t)nr_pages << PAGE_SHIFT),
> > -					    0, node, NULL);
> > -		if (!IS_ERR(etr_buf))
> > -			goto done;
> > -	}
> > +
> > +	/* Use the minimum limit if the required size is smaller */
> > +	size = (unsigned long)nr_pages << PAGE_SHIFT;
> Please change the size's type to ssize_t, then:
>
>          size = nr_pages << PAGE_SHIFT;
>
> > +	if (size < TMC_ETR_PERF_MIN_BUF_SIZE)
> > +		size = TMC_ETR_PERF_MIN_BUF_SIZE;
> size = min_t(ssize_t, size, TMC_ETR_PERF_MIN_BUF_SIZE);

Sure, I will do that.

> >   
> >   	/*
> > -	 * Else switch to configured size for this ETR
> > -	 * and scale down until we hit the minimum limit.
> > +	 * Try to allocate the required size for this ETR, if failed scale
> > +	 * down until we hit the minimum limit.
> >   	 */
> > -	size = drvdata->size;
> >   	do {
> >   		etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
> >   		if (!IS_ERR(etr_buf))
> > -			goto done;
> > +			return etr_buf;
> >   		size /= 2;
> >   	} while (size >= TMC_ETR_PERF_MIN_BUF_SIZE);
> Do we really need to scale down buffer size for failure cases?
> I would like a straightforward code:
>
>          etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
>          if (IS_ERR_OR_NULL(etr_buf))
>                  return etr_buf;
>
> Just a side topic, we know tmc_alloc_etr_buf() should not return NULL
> pointer. For a sanity check, the callers (alloc_etr_buf(),
> tmc_etr_get_sysfs_buffer(), etc) should valid a buffer pointer with
> IS_ERR_OR_NULL() rather than IS_ERR(). This can be a separate patch.

A new patch will be added to achieve this.

Thank you for your comments.
Junhao.

> Thanks,
> Leo
>
> >   	return ERR_PTR(-ENOMEM);
> > -
> > -done:
> > -	return etr_buf;
> >   }
> >   
> >   static struct etr_buf *
> > --
> > 2.33.0
> .