* [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used
2018-12-10 0:37 [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage Dongli Zhang
@ 2018-12-10 0:37 ` Dongli Zhang
2018-12-10 17:09 ` Joe Jin
2019-01-17 15:25 ` Konrad Rzeszutek Wilk
2018-12-10 17:10 ` [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage Joe Jin
2018-12-10 20:00 ` Tim Chen
2 siblings, 2 replies; 8+ messages in thread
From: Dongli Zhang @ 2018-12-10 0:37 UTC (permalink / raw)
To: iommu, linux-kernel
Cc: konrad.wilk, hch, m.szyprowski, robin.murphy, joe.jin,
dongli.zhang
This patch uses io_tlb_used to help check whether swiotlb buffer is full.
io_tlb_used is no longer used for only debugfs. It is also used to help
optimize swiotlb_tbl_map_single().
Suggested-by: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
kernel/dma/swiotlb.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 3979c2c..9300341 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -76,12 +76,10 @@ static phys_addr_t io_tlb_start, io_tlb_end;
*/
static unsigned long io_tlb_nslabs;
-#ifdef CONFIG_DEBUG_FS
/*
* The number of used IO TLB block
*/
static unsigned long io_tlb_used;
-#endif
/*
* This is a free list describing the number of free entries available from
@@ -489,6 +487,10 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
* request and allocate a buffer from that IO TLB pool.
*/
spin_lock_irqsave(&io_tlb_lock, flags);
+
+ if (unlikely(nslots > io_tlb_nslabs - io_tlb_used))
+ goto not_found;
+
index = ALIGN(io_tlb_index, stride);
if (index >= io_tlb_nslabs)
index = 0;
@@ -538,9 +540,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
return SWIOTLB_MAP_ERROR;
found:
-#ifdef CONFIG_DEBUG_FS
io_tlb_used += nslots;
-#endif
spin_unlock_irqrestore(&io_tlb_lock, flags);
/*
@@ -602,9 +602,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
io_tlb_list[i] = ++count;
-#ifdef CONFIG_DEBUG_FS
io_tlb_used -= nslots;
-#endif
}
spin_unlock_irqrestore(&io_tlb_lock, flags);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used
2018-12-10 0:37 ` [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used Dongli Zhang
@ 2018-12-10 17:09 ` Joe Jin
2019-01-17 15:25 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 8+ messages in thread
From: Joe Jin @ 2018-12-10 17:09 UTC (permalink / raw)
To: Dongli Zhang, iommu, linux-kernel
Cc: konrad.wilk, hch, m.szyprowski, robin.murphy
On 12/9/18 4:37 PM, Dongli Zhang wrote:
> This patch uses io_tlb_used to help check whether swiotlb buffer is full.
> io_tlb_used is no longer used for only debugfs. It is also used to help
> optimize swiotlb_tbl_map_single().
>
> Suggested-by: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
Reviewed-by: Joe Jin <joe.jin@oracle.com>
> ---
> kernel/dma/swiotlb.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 3979c2c..9300341 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -76,12 +76,10 @@ static phys_addr_t io_tlb_start, io_tlb_end;
> */
> static unsigned long io_tlb_nslabs;
>
> -#ifdef CONFIG_DEBUG_FS
> /*
> * The number of used IO TLB block
> */
> static unsigned long io_tlb_used;
> -#endif
>
> /*
> * This is a free list describing the number of free entries available from
> @@ -489,6 +487,10 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> * request and allocate a buffer from that IO TLB pool.
> */
> spin_lock_irqsave(&io_tlb_lock, flags);
> +
> + if (unlikely(nslots > io_tlb_nslabs - io_tlb_used))
> + goto not_found;
> +
> index = ALIGN(io_tlb_index, stride);
> if (index >= io_tlb_nslabs)
> index = 0;
> @@ -538,9 +540,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
> return SWIOTLB_MAP_ERROR;
> found:
> -#ifdef CONFIG_DEBUG_FS
> io_tlb_used += nslots;
> -#endif
> spin_unlock_irqrestore(&io_tlb_lock, flags);
>
> /*
> @@ -602,9 +602,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
> for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> io_tlb_list[i] = ++count;
>
> -#ifdef CONFIG_DEBUG_FS
> io_tlb_used -= nslots;
> -#endif
> }
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> }
>
--
Oracle <http://www.oracle.com>
Joe Jin | Software Development Director
ORACLE | Linux and Virtualization
500 Oracle Parkway Redwood City, CA US 94065
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used
2018-12-10 0:37 ` [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used Dongli Zhang
2018-12-10 17:09 ` Joe Jin
@ 2019-01-17 15:25 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2019-01-17 15:25 UTC (permalink / raw)
To: Dongli Zhang
Cc: iommu, linux-kernel, konrad.wilk, hch, m.szyprowski, robin.murphy,
joe.jin
On Mon, Dec 10, 2018 at 08:37:58AM +0800, Dongli Zhang wrote:
> This patch uses io_tlb_used to help check whether swiotlb buffer is full.
> io_tlb_used is no longer used for only debugfs. It is also used to help
> optimize swiotlb_tbl_map_single().
Please split this up.
That is have the 'if (unlikely(nslots > io_tlb_nslabs - io_tlb_used))'
as a seperate patch.
And the #ifdef folding in the previous patch.
Also rebase on top of latest Linus please.
>
> Suggested-by: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> kernel/dma/swiotlb.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 3979c2c..9300341 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -76,12 +76,10 @@ static phys_addr_t io_tlb_start, io_tlb_end;
> */
> static unsigned long io_tlb_nslabs;
>
> -#ifdef CONFIG_DEBUG_FS
> /*
> * The number of used IO TLB block
> */
> static unsigned long io_tlb_used;
> -#endif
>
> /*
> * This is a free list describing the number of free entries available from
> @@ -489,6 +487,10 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> * request and allocate a buffer from that IO TLB pool.
> */
> spin_lock_irqsave(&io_tlb_lock, flags);
> +
> + if (unlikely(nslots > io_tlb_nslabs - io_tlb_used))
> + goto not_found;
> +
> index = ALIGN(io_tlb_index, stride);
> if (index >= io_tlb_nslabs)
> index = 0;
> @@ -538,9 +540,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
> return SWIOTLB_MAP_ERROR;
> found:
> -#ifdef CONFIG_DEBUG_FS
> io_tlb_used += nslots;
> -#endif
> spin_unlock_irqrestore(&io_tlb_lock, flags);
>
> /*
> @@ -602,9 +602,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
> for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> io_tlb_list[i] = ++count;
>
> -#ifdef CONFIG_DEBUG_FS
> io_tlb_used -= nslots;
> -#endif
> }
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage
2018-12-10 0:37 [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage Dongli Zhang
2018-12-10 0:37 ` [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used Dongli Zhang
@ 2018-12-10 17:10 ` Joe Jin
2018-12-10 20:00 ` Tim Chen
2 siblings, 0 replies; 8+ messages in thread
From: Joe Jin @ 2018-12-10 17:10 UTC (permalink / raw)
To: Dongli Zhang, iommu, linux-kernel
Cc: konrad.wilk, hch, m.szyprowski, robin.murphy
On 12/9/18 4:37 PM, Dongli Zhang wrote:
> The device driver will not be able to do dma operations once swiotlb buffer
> is full, either because the driver is using so many IO TLB blocks inflight,
> or because there is memory leak issue in device driver. To export the
> swiotlb buffer usage via debugfs would help the user estimate the size of
> swiotlb buffer to pre-allocate or analyze device driver memory leak issue.
>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
Reviewed-by: Joe Jin <joe.jin@oracle.com>
> ---
> Changed since v1:
> * init debugfs with late_initcall (suggested by Robin Murphy)
> * create debugfs entries with debugfs_create_ulong(suggested by Robin Murphy)
>
> kernel/dma/swiotlb.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 045930e..3979c2c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -35,6 +35,9 @@
> #include <linux/scatterlist.h>
> #include <linux/mem_encrypt.h>
> #include <linux/set_memory.h>
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/debugfs.h>
> +#endif
>
> #include <asm/io.h>
> #include <asm/dma.h>
> @@ -73,6 +76,13 @@ static phys_addr_t io_tlb_start, io_tlb_end;
> */
> static unsigned long io_tlb_nslabs;
>
> +#ifdef CONFIG_DEBUG_FS
> +/*
> + * The number of used IO TLB block
> + */
> +static unsigned long io_tlb_used;
> +#endif
> +
> /*
> * This is a free list describing the number of free entries available from
> * each index
> @@ -528,6 +538,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
> return SWIOTLB_MAP_ERROR;
> found:
> +#ifdef CONFIG_DEBUG_FS
> + io_tlb_used += nslots;
> +#endif
> spin_unlock_irqrestore(&io_tlb_lock, flags);
>
> /*
> @@ -588,6 +601,10 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
> */
> for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> io_tlb_list[i] = ++count;
> +
> +#ifdef CONFIG_DEBUG_FS
> + io_tlb_used -= nslots;
> +#endif
> }
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> }
> @@ -883,3 +900,36 @@ const struct dma_map_ops swiotlb_dma_ops = {
> .dma_supported = dma_direct_supported,
> };
> EXPORT_SYMBOL(swiotlb_dma_ops);
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int __init swiotlb_create_debugfs(void)
> +{
> + static struct dentry *d_swiotlb_usage;
> + struct dentry *ent;
> +
> + d_swiotlb_usage = debugfs_create_dir("swiotlb", NULL);
> +
> + if (!d_swiotlb_usage)
> + return -ENOMEM;
> +
> + ent = debugfs_create_ulong("io_tlb_nslabs", 0400,
> + d_swiotlb_usage, &io_tlb_nslabs);
> + if (!ent)
> + goto fail;
> +
> + ent = debugfs_create_ulong("io_tlb_used", 0400,
> + d_swiotlb_usage, &io_tlb_used);
> + if (!ent)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + debugfs_remove_recursive(d_swiotlb_usage);
> + return -ENOMEM;
> +}
> +
> +late_initcall(swiotlb_create_debugfs);
> +
> +#endif
>
--
Oracle <http://www.oracle.com>
Joe Jin | Software Development Director
ORACLE | Linux and Virtualization
500 Oracle Parkway Redwood City, CA US 94065
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage
2018-12-10 0:37 [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage Dongli Zhang
2018-12-10 0:37 ` [PATCH v2 2/2] swiotlb: checking whether swiotlb buffer is full with io_tlb_used Dongli Zhang
2018-12-10 17:10 ` [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage Joe Jin
@ 2018-12-10 20:00 ` Tim Chen
2018-12-10 21:05 ` Joe Jin
2 siblings, 1 reply; 8+ messages in thread
From: Tim Chen @ 2018-12-10 20:00 UTC (permalink / raw)
To: Dongli Zhang, iommu, linux-kernel
Cc: konrad.wilk, hch, m.szyprowski, robin.murphy, joe.jin
On 12/9/18 4:37 PM, Dongli Zhang wrote:
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 045930e..3979c2c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -35,6 +35,9 @@
> #include <linux/scatterlist.h>
> #include <linux/mem_encrypt.h>
> #include <linux/set_memory.h>
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/debugfs.h>
> +#endif
Seems like #ifdef CONFIG_DEBUG_FS is better located inside debugfs.h,
instead of requiring every file that includes it
to have a #ifdef CONFIG_DEBUG_FS.
>
> #include <asm/io.h>
> #include <asm/dma.h>
> @@ -73,6 +76,13 @@ static phys_addr_t io_tlb_start, io_tlb_end;
> */
> static unsigned long io_tlb_nslabs;
>
> +#ifdef CONFIG_DEBUG_FS
> +/*
> + * The number of used IO TLB block
> + */
> +static unsigned long io_tlb_used;
> +#endif
> +
> /*
> * This is a free list describing the number of free entries available from
> * each index
> @@ -528,6 +538,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
> return SWIOTLB_MAP_ERROR;
> found:
> +#ifdef CONFIG_DEBUG_FS
> + io_tlb_used += nslots;
> +#endif
One nit I have about this patch is there are too many CONFIG_DEBUG_FS.
For example here, instead of io_tlb_used, we can have a macro defined,
perhaps something like inc_iotlb_used(nslots). It can be placed in the
same section that swiotlb_create_debugfs is defined so there's a single
place where all the CONFIG_DEBUG_FS stuff is located.
Then define inc_iotlb_used to be null when we don't have
CONFIG_DEBUG_FS.
> spin_unlock_irqrestore(&io_tlb_lock, flags);
>
> /*
> @@ -588,6 +601,10 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
> */
> for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> io_tlb_list[i] = ++count;
> +
> +#ifdef CONFIG_DEBUG_FS
> + io_tlb_used -= nslots;
> +#endif
> }
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> }
> @@ -883,3 +900,36 @@ const struct dma_map_ops swiotlb_dma_ops = {
> .dma_supported = dma_direct_supported,
> };
> EXPORT_SYMBOL(swiotlb_dma_ops);
> +
> +#ifdef CONFIG_DEBUG_FS
> +
Maybe move io_tlb_used definition here and define
inc_iotlb_used here.
> +static int __init swiotlb_create_debugfs(void)
> +{
> + static struct dentry *d_swiotlb_usage;
> + struct dentry *ent;
> +
> + d_swiotlb_usage = debugfs_create_dir("swiotlb", NULL);
> +
> + if (!d_swiotlb_usage)
> + return -ENOMEM;
> +
> + ent = debugfs_create_ulong("io_tlb_nslabs", 0400,
> + d_swiotlb_usage, &io_tlb_nslabs);
> + if (!ent)
> + goto fail;
> +
> + ent = debugfs_create_ulong("io_tlb_used", 0400,
> + d_swiotlb_usage, &io_tlb_used);
> + if (!ent)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + debugfs_remove_recursive(d_swiotlb_usage);
> + return -ENOMEM;
> +}
> +
> +late_initcall(swiotlb_create_debugfs);
> +
> +#endif
>
Thanks.
Tim
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage
2018-12-10 20:00 ` Tim Chen
@ 2018-12-10 21:05 ` Joe Jin
2019-01-04 1:34 ` Dongli Zhang
0 siblings, 1 reply; 8+ messages in thread
From: Joe Jin @ 2018-12-10 21:05 UTC (permalink / raw)
To: Tim Chen, Dongli Zhang, iommu, linux-kernel
Cc: konrad.wilk, hch, m.szyprowski, robin.murphy
On 12/10/18 12:00 PM, Tim Chen wrote:
>> @@ -528,6 +538,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
>> return SWIOTLB_MAP_ERROR;
>> found:
>> +#ifdef CONFIG_DEBUG_FS
>> + io_tlb_used += nslots;
>> +#endif
> One nit I have about this patch is there are too many CONFIG_DEBUG_FS.
>
> For example here, instead of io_tlb_used, we can have a macro defined,
> perhaps something like inc_iotlb_used(nslots). It can be placed in the
> same section that swiotlb_create_debugfs is defined so there's a single
> place where all the CONFIG_DEBUG_FS stuff is located.
>
> Then define inc_iotlb_used to be null when we don't have
> CONFIG_DEBUG_FS.
>
Dongli had removed above ifdef/endif on his next patch, "[PATCH v2 2/2]
swiotlb: checking whether swiotlb buffer is full with io_tlb_used"
Thanks,
Joe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] swiotlb: add debugfs to track swiotlb buffer usage
2018-12-10 21:05 ` Joe Jin
@ 2019-01-04 1:34 ` Dongli Zhang
0 siblings, 0 replies; 8+ messages in thread
From: Dongli Zhang @ 2019-01-04 1:34 UTC (permalink / raw)
To: iommu, linux-kernel, konrad.wilk
Cc: Joe Jin, Tim Chen, hch, m.szyprowski, robin.murphy
Hi Konrad,
Would you please take a look on those two patches?
In addition, there is another patch correcting an error in comment.
https://lkml.org/lkml/2018/12/5/1721
Thank you very much!
Dongli Zhang
On 12/11/2018 05:05 AM, Joe Jin wrote:
> On 12/10/18 12:00 PM, Tim Chen wrote:
>>> @@ -528,6 +538,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>>> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n", size);
>>> return SWIOTLB_MAP_ERROR;
>>> found:
>>> +#ifdef CONFIG_DEBUG_FS
>>> + io_tlb_used += nslots;
>>> +#endif
>> One nit I have about this patch is there are too many CONFIG_DEBUG_FS.
>>
>> For example here, instead of io_tlb_used, we can have a macro defined,
>> perhaps something like inc_iotlb_used(nslots). It can be placed in the
>> same section that swiotlb_create_debugfs is defined so there's a single
>> place where all the CONFIG_DEBUG_FS stuff is located.
>>
>> Then define inc_iotlb_used to be null when we don't have
>> CONFIG_DEBUG_FS.
>>
>
> Dongli had removed above ifdef/endif on his next patch, "[PATCH v2 2/2]
> swiotlb: checking whether swiotlb buffer is full with io_tlb_used"
>
> Thanks,
> Joe
>
^ permalink raw reply [flat|nested] 8+ messages in thread