[RFC PATCH v1] Trying to fix the stmmac memory leak during suspend/resume

[RFC PATCH v1] Trying to fix the stmmac memory leak during suspend/resume

[Shunqian Zheng]

From: ZhengShunQian <zhengsq@rock-chips.com>

When I run Suspend-to-Ram stress test on my Rockchip RK3288(SoC) board
that integrated stmmac ethernet, it always OOM after a few iterations,
usually 50 times is enough to reproduce.

Compiled kernel with KMEMLEAK feature, I got the logs as below:
unreferenced object 0xed89ac00 (size 192):
  comm "busybox", pid 79, jiffies 2251 (age 54.580s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 d1 ed 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<c05430dc>] kmemleak_alloc+0x44/0x78
    [<c012b198>] kmem_cache_alloc+0x1ac/0x264
    [<c0442134>] __build_skb+0x38/0x9c
    [<c0442350>] __netdev_alloc_skb+0xac/0x118
    [<c0335f8c>] init_dma_desc_rings+0xcc/0x474
    [<c0336f20>] stmmac_resume+0xc4/0x14c
    [<c033ed28>] stmmac_pltfr_resume+0x3c/0x40
    [<c02ed4dc>] platform_pm_resume+0x3c/0x50
    [<c02f7ad0>] dpm_run_callback+0x7c/0x160
    [<c02f7e14>] device_resume+0x174/0x1c0
    [<c02f9368>] dpm_resume+0x110/0x2cc
    [<c02f9830>] dpm_resume_end+0x1c/0x28
    [<c00646c0>] suspend_devices_and_enter+0x53c/0x6ec
    [<c0064ba4>] pm_suspend+0x334/0x478
    [<c0063110>] state_store+0xac/0xc8
    [<c0265228>] kobj_attr_store+0x1c/0x28

Actually I don't think I know net/stmmac good enough to fix this bug.
I really appreciate that the exports of net/stmmac can take over it if
you think it is a bug too.

ZhengShunQian (1):
  net: stmmac: Free rx_skbufs before realloc

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
1.9.1

[PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Shunqian Zheng]

From: ZhengShunQian <zhengsq@rock-chips.com>

The init_dma_desc_rings() may realloc the rx_skbuff[] when
suspend and resume. This patch free the rx_skbuff[] before
reallocing memory.

Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 64d8aa4..2af1ed9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1022,6 +1022,14 @@ static void stmmac_free_rx_buffers(struct stmmac_priv *priv, int i)
 	priv->rx_skbuff[i] = NULL;
 }
 
+static void dma_free_rx_skbufs(struct stmmac_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->dma_rx_size; i++)
+		stmmac_free_rx_buffers(priv, i);
+}
+
 /**
  * init_dma_desc_rings - init the RX/TX descriptor rings
  * @dev: net device structure
@@ -1058,6 +1066,8 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
 		/* RX INITIALIZATION */
 		pr_debug("\tSKB addresses:\nskb\t\tskb data\tdma data\n");
 	}
+
+	dma_free_rx_skbufs(priv);
 	for (i = 0; i < rxsize; i++) {
 		struct dma_desc *p;
 		if (priv->extend_desc)
@@ -1122,14 +1132,6 @@ err_init_rx_buffers:
 	return ret;
 }
 
-static void dma_free_rx_skbufs(struct stmmac_priv *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->dma_rx_size; i++)
-		stmmac_free_rx_buffers(priv, i);
-}
-
 static void dma_free_tx_skbufs(struct stmmac_priv *priv)
 {
 	int i;
-- 
1.9.1

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[David Miller]

From: Shunqian Zheng <zhengsq@rock-chips.com>
Date: Sun, 22 Nov 2015 16:44:18 +0800

> From: ZhengShunQian <zhengsq@rock-chips.com>
> 
> The init_dma_desc_rings() may realloc the rx_skbuff[] when
> suspend and resume. This patch free the rx_skbuff[] before
> reallocing memory.
> 
> Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com>

This isn't really the right way to fix this.

I see two reasonable approaches:

1) suspend liberates the RX ring, although this approach is less
   desirable

2) resume doesn't try to allocate already populated RX ring
   entries

Freeing the whole RX ring just to allocate it again immediately
makes no sense at all and is wasteful work.

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

Hello

On 11/24/2015 7:09 PM, David Miller wrote:
> From: Shunqian Zheng <zhengsq@rock-chips.com>
> Date: Sun, 22 Nov 2015 16:44:18 +0800
>
>> From: ZhengShunQian <zhengsq@rock-chips.com>
>>
>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>> suspend and resume. This patch free the rx_skbuff[] before
>> reallocing memory.
>>
>> Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com>
>
> This isn't really the right way to fix this.
>
> I see two reasonable approaches:
>
> 1) suspend liberates the RX ring, although this approach is less
>     desirable
>
> 2) resume doesn't try to allocate already populated RX ring
>     entries
>
> Freeing the whole RX ring just to allocate it again immediately
> makes no sense at all and is wasteful work.

This is a bug in this driver version that, to be honest, we fixed with
the first approach on STi kernel.
The patch just called the dma_free_rx_skbufs(priv) in the suspend.
I can give you this patch that is tested on my side too.
But! I do think we should move on second approach.
Indeed, also on ST platforms, when we play with suspend states
the DDR although in self-refresh the data are not lost at all.
No reason to free and reallocate all in suspend/resume.
I can test that and then provide another patch to this mailing list
asap.

Let me know.
peppe

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

Hello

On 11/24/2015 7:09 PM, David Miller wrote:
> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Date: Sun, 22 Nov 2015 16:44:18 +0800
>
>> From: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>> suspend and resume. This patch free the rx_skbuff[] before
>> reallocing memory.
>>
>> Signed-off-by: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> This isn't really the right way to fix this.
>
> I see two reasonable approaches:
>
> 1) suspend liberates the RX ring, although this approach is less
>     desirable
>
> 2) resume doesn't try to allocate already populated RX ring
>     entries
>
> Freeing the whole RX ring just to allocate it again immediately
> makes no sense at all and is wasteful work.

This is a bug in this driver version that, to be honest, we fixed with
the first approach on STi kernel.
The patch just called the dma_free_rx_skbufs(priv) in the suspend.
I can give you this patch that is tested on my side too.
But! I do think we should move on second approach.
Indeed, also on ST platforms, when we play with suspend states
the DDR although in self-refresh the data are not lost at all.
No reason to free and reallocate all in suspend/resume.
I can test that and then provide another patch to this mailing list
asap.

Let me know.
peppe

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

Hello

On 11/24/2015 7:09 PM, David Miller wrote:
> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Date: Sun, 22 Nov 2015 16:44:18 +0800
>
>> From: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>> suspend and resume. This patch free the rx_skbuff[] before
>> reallocing memory.
>>
>> Signed-off-by: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> This isn't really the right way to fix this.
>
> I see two reasonable approaches:
>
> 1) suspend liberates the RX ring, although this approach is less
>     desirable
>
> 2) resume doesn't try to allocate already populated RX ring
>     entries
>
> Freeing the whole RX ring just to allocate it again immediately
> makes no sense at all and is wasteful work.

This is a bug in this driver version that, to be honest, we fixed with
the first approach on STi kernel.
The patch just called the dma_free_rx_skbufs(priv) in the suspend.
I can give you this patch that is tested on my side too.
But! I do think we should move on second approach.
Indeed, also on ST platforms, when we play with suspend states
the DDR although in self-refresh the data are not lost at all.
No reason to free and reallocate all in suspend/resume.
I can test that and then provide another patch to this mailing list
asap.

Let me know.
peppe

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

On 11/25/2015 4:13 PM, Giuseppe CAVALLARO wrote:
> Hello
>
> On 11/24/2015 7:09 PM, David Miller wrote:
>> From: Shunqian Zheng <zhengsq@rock-chips.com>
>> Date: Sun, 22 Nov 2015 16:44:18 +0800
>>
>>> From: ZhengShunQian <zhengsq@rock-chips.com>
>>>
>>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>>> suspend and resume. This patch free the rx_skbuff[] before
>>> reallocing memory.
>>>
>>> Signed-off-by: ZhengShunQian <zhengsq@rock-chips.com>
>>
>> This isn't really the right way to fix this.
>>
>> I see two reasonable approaches:
>>
>> 1) suspend liberates the RX ring, although this approach is less
>>     desirable
>>
>> 2) resume doesn't try to allocate already populated RX ring
>>     entries
>>
>> Freeing the whole RX ring just to allocate it again immediately
>> makes no sense at all and is wasteful work.
>
> This is a bug in this driver version that, to be honest, we fixed with
> the first approach on STi kernel.
> The patch just called the dma_free_rx_skbufs(priv) in the suspend.
> I can give you this patch that is tested on my side too.
> But! I do think we should move on second approach.
> Indeed, also on ST platforms, when we play with suspend states
> the DDR although in self-refresh the data are not lost at all.
> No reason to free and reallocate all in suspend/resume.
> I can test that and then provide another patch to this mailing list
> asap.

I have just send the patch (directly for approach #2).

Peppe

>
> Let me know.
> peppe
>
>
>

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

On 11/25/2015 4:13 PM, Giuseppe CAVALLARO wrote:
> Hello
>
> On 11/24/2015 7:09 PM, David Miller wrote:
>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Date: Sun, 22 Nov 2015 16:44:18 +0800
>>
>>> From: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>
>>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>>> suspend and resume. This patch free the rx_skbuff[] before
>>> reallocing memory.
>>>
>>> Signed-off-by: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> This isn't really the right way to fix this.
>>
>> I see two reasonable approaches:
>>
>> 1) suspend liberates the RX ring, although this approach is less
>>     desirable
>>
>> 2) resume doesn't try to allocate already populated RX ring
>>     entries
>>
>> Freeing the whole RX ring just to allocate it again immediately
>> makes no sense at all and is wasteful work.
>
> This is a bug in this driver version that, to be honest, we fixed with
> the first approach on STi kernel.
> The patch just called the dma_free_rx_skbufs(priv) in the suspend.
> I can give you this patch that is tested on my side too.
> But! I do think we should move on second approach.
> Indeed, also on ST platforms, when we play with suspend states
> the DDR although in self-refresh the data are not lost at all.
> No reason to free and reallocate all in suspend/resume.
> I can test that and then provide another patch to this mailing list
> asap.

I have just send the patch (directly for approach #2).

Peppe

>
> Let me know.
> peppe
>
>
>

Re: [PATCH v1] net: stmmac: Free rx_skbufs before realloc

[Giuseppe CAVALLARO]

On 11/25/2015 4:13 PM, Giuseppe CAVALLARO wrote:
> Hello
>
> On 11/24/2015 7:09 PM, David Miller wrote:
>> From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Date: Sun, 22 Nov 2015 16:44:18 +0800
>>
>>> From: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>
>>> The init_dma_desc_rings() may realloc the rx_skbuff[] when
>>> suspend and resume. This patch free the rx_skbuff[] before
>>> reallocing memory.
>>>
>>> Signed-off-by: ZhengShunQian <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> This isn't really the right way to fix this.
>>
>> I see two reasonable approaches:
>>
>> 1) suspend liberates the RX ring, although this approach is less
>>     desirable
>>
>> 2) resume doesn't try to allocate already populated RX ring
>>     entries
>>
>> Freeing the whole RX ring just to allocate it again immediately
>> makes no sense at all and is wasteful work.
>
> This is a bug in this driver version that, to be honest, we fixed with
> the first approach on STi kernel.
> The patch just called the dma_free_rx_skbufs(priv) in the suspend.
> I can give you this patch that is tested on my side too.
> But! I do think we should move on second approach.
> Indeed, also on ST platforms, when we play with suspend states
> the DDR although in self-refresh the data are not lost at all.
> No reason to free and reallocate all in suspend/resume.
> I can test that and then provide another patch to this mailing list
> asap.

I have just send the patch (directly for approach #2).

Peppe

>
> Let me know.
> peppe
>
>
>

[PATCH (net.git)] stmmac: fix resource management when resume

[Giuseppe Cavallaro]

There is a memleak when suspend/resume this driver version.
Currently the stmmac, during resume step, reallocates all the resources
but they are not released when suspend.
The patch is not to release these resources but the logic has been changed.
In fact, it is not necessary to free and reallocate all from scratch
because the memory data will be always preserved.
As final solution, the patch just reinit the descriptors and the rx/tx
pointers only when resume. Tested done on STi boxes.

Reported-by: ZhengShunQian <zhengsq@rock-chips.com>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 64d8aa4..5a1abc0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3040,8 +3040,6 @@ int stmmac_suspend(struct net_device *ndev)
 	priv->hw->dma->stop_tx(priv->ioaddr);
 	priv->hw->dma->stop_rx(priv->ioaddr);
 
-	stmmac_clear_descriptors(priv);
-
 	/* Enable Power down mode by programming the PMT regs */
 	if (device_may_wakeup(priv->device)) {
 		priv->hw->mac->pmt(priv->hw, priv->wolopts);
@@ -3099,7 +3097,12 @@ int stmmac_resume(struct net_device *ndev)
 
 	netif_device_attach(ndev);
 
-	init_dma_desc_rings(ndev, GFP_ATOMIC);
+	priv->cur_rx = 0;
+	priv->dirty_rx = 0;
+	priv->dirty_tx = 0;
+	priv->cur_tx = 0;
+	stmmac_clear_descriptors(priv);
+
 	stmmac_hw_setup(ndev, false);
 	stmmac_init_tx_coalesce(priv);
 
-- 
1.7.4.4

Re: [PATCH (net.git)] stmmac: fix resource management when resume

[David Miller]

From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Thu, 26 Nov 2015 11:32:04 +0100

> There is a memleak when suspend/resume this driver version.
> Currently the stmmac, during resume step, reallocates all the resources
> but they are not released when suspend.
> The patch is not to release these resources but the logic has been changed.
> In fact, it is not necessary to free and reallocate all from scratch
> because the memory data will be always preserved.
> As final solution, the patch just reinit the descriptors and the rx/tx
> pointers only when resume. Tested done on STi boxes.
> 
> Reported-by: ZhengShunQian <zhengsq@rock-chips.com>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

This doesn't apply to the 'net' tree.

Re: [PATCH (net.git)] stmmac: fix resource management when resume

[David Miller]

From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Fri, 4 Dec 2015 07:21:06 +0100

> There is a memleak when suspend/resume this driver version.
> Currently the stmmac, during resume step, reallocates all the resources
> but they are not released when suspend.
> The patch is not to release these resources but the logic has been changed.
> In fact, it is not necessary to free and reallocate all from scratch
> because the memory data will be always preserved.
> As final solution, the patch just reinit the descriptors and the rx/tx
> pointers only when resume. Tested done on STi boxes.
> 
> Reported-by: ZhengShunQian <zhengsq@rock-chips.com>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

Applied, thanks.

Re: [PATCH (net.git)] stmmac: fix resource management when resume

[Giuseppe CAVALLARO]

Hi David

This is the v2, sorry I missed it in the subject.

Re-based on top of net.git.

peppe

On 12/4/2015 7:21 AM, Giuseppe Cavallaro wrote:
> There is a memleak when suspend/resume this driver version.
> Currently the stmmac, during resume step, reallocates all the resources
> but they are not released when suspend.
> The patch is not to release these resources but the logic has been changed.
> In fact, it is not necessary to free and reallocate all from scratch
> because the memory data will be always preserved.
> As final solution, the patch just reinit the descriptors and the rx/tx
> pointers only when resume. Tested done on STi boxes.
>
> Reported-by: ZhengShunQian <zhengsq@rock-chips.com>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 ++++++---
>   1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3c6549a..a5b869e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3046,8 +3046,6 @@ int stmmac_suspend(struct net_device *ndev)
>   	priv->hw->dma->stop_tx(priv->ioaddr);
>   	priv->hw->dma->stop_rx(priv->ioaddr);
>
> -	stmmac_clear_descriptors(priv);
> -
>   	/* Enable Power down mode by programming the PMT regs */
>   	if (device_may_wakeup(priv->device)) {
>   		priv->hw->mac->pmt(priv->hw, priv->wolopts);
> @@ -3105,7 +3103,12 @@ int stmmac_resume(struct net_device *ndev)
>
>   	netif_device_attach(ndev);
>
> -	init_dma_desc_rings(ndev, GFP_ATOMIC);
> +	priv->cur_rx = 0;
> +	priv->dirty_rx = 0;
> +	priv->dirty_tx = 0;
> +	priv->cur_tx = 0;
> +	stmmac_clear_descriptors(priv);
> +
>   	stmmac_hw_setup(ndev, false);
>   	stmmac_init_tx_coalesce(priv);
>   	stmmac_set_rx_mode(ndev);
>

[PATCH (net.git)] stmmac: fix resource management when resume

[Giuseppe Cavallaro]

There is a memleak when suspend/resume this driver version.
Currently the stmmac, during resume step, reallocates all the resources
but they are not released when suspend.
The patch is not to release these resources but the logic has been changed.
In fact, it is not necessary to free and reallocate all from scratch
because the memory data will be always preserved.
As final solution, the patch just reinit the descriptors and the rx/tx
pointers only when resume. Tested done on STi boxes.

Reported-by: ZhengShunQian <zhengsq@rock-chips.com>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3c6549a..a5b869e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3046,8 +3046,6 @@ int stmmac_suspend(struct net_device *ndev)
 	priv->hw->dma->stop_tx(priv->ioaddr);
 	priv->hw->dma->stop_rx(priv->ioaddr);
 
-	stmmac_clear_descriptors(priv);
-
 	/* Enable Power down mode by programming the PMT regs */
 	if (device_may_wakeup(priv->device)) {
 		priv->hw->mac->pmt(priv->hw, priv->wolopts);
@@ -3105,7 +3103,12 @@ int stmmac_resume(struct net_device *ndev)
 
 	netif_device_attach(ndev);
 
-	init_dma_desc_rings(ndev, GFP_ATOMIC);
+	priv->cur_rx = 0;
+	priv->dirty_rx = 0;
+	priv->dirty_tx = 0;
+	priv->cur_tx = 0;
+	stmmac_clear_descriptors(priv);
+
 	stmmac_hw_setup(ndev, false);
 	stmmac_init_tx_coalesce(priv);
 	stmmac_set_rx_mode(ndev);
-- 
1.7.4.4