Re: [PATCH V2 07/15] coresight: tmc: allocating memory when needed

[Suzuki K Poulose]

On 12/04/16 18:54, Mathieu Poirier wrote:
> In it's current form the TMC probe() function allocates
> trace buffer memory at boot time, event if coresight isn't
> used.  This is highly inefficient since trace buffers can
> occupy a lot of memory that could be used otherwised.
>
> This patch allocates trace buffers on the fly, when the
> coresight subsystem is solicited.  Allocated buffers are
> released when traces are read using the device descriptors
> under /dev.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> ---
>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 85 +++++++++++++++++++++++--
>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 83 +++++++++++++++++++++++-
>   drivers/hwtracing/coresight/coresight-tmc.c     | 14 ----
>   3 files changed, 163 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 4b8f39bd478b..7cb287ef7b9e 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -16,14 +16,12 @@
>    */
>
>   #include <linux/coresight.h>
> +#include <linux/slab.h>
>   #include "coresight-priv.h"
>   #include "coresight-tmc.h"
>
>   void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>   {
> -	/* Zero out the memory to help with debug */
> -	memset(drvdata->buf, 0, drvdata->size);
> -
>   	CS_UNLOCK(drvdata->base);
>
>   	/* Wait for TMCSReady bit to be set */
> @@ -110,19 +108,68 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
>
>   static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>   {
> +	bool allocated = false;

nit: does "used" or buf_used sound more suitable than allocated ?

> +	char *buf = NULL;
>   	unsigned long flags;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> +	 /* This shouldn't be happening */
> +	WARN_ON(mode != CS_MODE_SYSFS);
> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->buf)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/* Allocating the memory here while outside of the spinlock */
> +	buf = kzalloc(drvdata->size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:
>   	if (drvdata->reading) {
>   		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		/*
> +		 * Free allocated memory outside of the spinlock.  There is
> +		 * no need to assert the validity of 'buf' since calling
> +		 * kfree(NULL) is safe.
> +		 */
> +		kfree(buf);
>   		return -EBUSY;
>   	}

We could check do the above check, before the allocation and avoid an unnecessary
alloc/free() if we really don't need that. And may be its better to get rid of the
"jump to fastpath" to avoid complicating the code, by using something like :

	lock();
	if (drvdata->reading) {
		rc = -EBUSY;
		goto unlock_out;
	}

	if (!drvdata->buf) {
	/* Drop the lock here before allocation and retake the lock */
		unlock();
		alloc();
		lock();
		if (!buf) {
			rc = -ENOMEM;
			goto unlock_out;
		}
	}
...

> +
>   	tmc_etb_enable_hw(drvdata);
>   	drvdata->enable = true;

unlock_out:

>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> +	/* Free memory outside the spinlock if need be */
> +	if (!allocated && buf)
> +		kfree(buf);
> +



> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 495540e9064d..6022ff26deba 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -16,6 +16,7 @@
>    */
>
>   #include <linux/coresight.h>
> +#include <linux/dma-mapping.h>
>   #include "coresight-priv.h"
>   #include "coresight-tmc.h"
>
> @@ -83,19 +84,69 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
>
>   static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>   {

> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->vaddr)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/*
> +	 * Contiguous  memory can't be allocated while a spinlock is held.
> +	 * As such allocate memory here and free it if a buffer has already
> +	 * been allocated (from a previous session).
> +	 */
> +	vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
> +				   &paddr, GFP_KERNEL);
> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:
>   	if (drvdata->reading) {
>   		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		if (vaddr)
> +			dma_free_coherent(drvdata->dev, drvdata->size,
> +					  vaddr, paddr);
>   		return -EBUSY;
>   	}

Same as above, if you move the check above before allocation, we could avoid
the alloc/free for such cases. And it would be better if simplify the code without
using the fast_path label to the middle of the code.

Otherwise, looks good.

Thanks
Suzuki