Hi Benn,<br><br> Please find my comments below.<br><br><div class="gmail_quote">On Tue, Jun 24, 2008 at 4:51 AM, Benjamin Herrenschmidt <<a href="mailto:benh@kernel.crashing.org">benh@kernel.crashing.org</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d">On Mon, 2008-06-23 at 14:55 +0200, Stefan Roese wrote:<br>
> From: Sathya Narayanan <<a href="mailto:sathyan@teamf1.com">sathyan@teamf1.com</a>><br>
><br>
> This patch addresses the memory leak happenning in drivers transmit queue<br>
> under heavy load condition. Once the transmit queue becomes full, driver<br>
> does an automatic wrapup of queue. During which the untransmitted SKB's are<br>
> lost without getting freed up.<br>
<br>
</div>This would be a bug. We should stop the queue when full instead.</blockquote><div><br>Actually the meachanism of stopping the queue and starting it is
already there. But even then due to some sync issue between the poll
routine and xmit, we were resulted in using the slots of skb which was
not actually got freed before. <br>I agree this could a bug , Since its not is not clear why buffers are not getting transferred timely?. But to handle this we should have a work around otherwise system may go out of memory. If we go for stopping the queue in these scenario also ( Where a unfreed skbs slot has been assigned to another ), Then kernel may call tx timeout, And reset the driver. In that case handelling this special case here could lead us better performance as compared to stopping the queue<br>
Let me know your comments.<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<div><div></div><div class="Wj3C7c"><br>
> Signed-off-by: Sathya Narayanan <<a href="mailto:sathyan@teamf1.com">sathyan@teamf1.com</a>><br>
> Signed-off-by: Stefan Roese <<a href="mailto:sr@denx.de">sr@denx.de</a>><br>
> ---<br>
> drivers/net/ibm_newemac/core.c | 7 +++++++<br>
> 1 files changed, 7 insertions(+), 0 deletions(-)<br>
><br>
> diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c<br>
> index aa407b2..ee868b6 100644<br>
> --- a/drivers/net/ibm_newemac/core.c<br>
> +++ b/drivers/net/ibm_newemac/core.c<br>
> @@ -1328,6 +1328,13 @@ static int emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)<br>
><br>
> DBG2(dev, "xmit(%u) %d" NL, len, slot);<br>
><br>
> + if (dev->tx_skb[slot] && dev->tx_desc[slot].data_ptr) {<br>
> + dev_kfree_skb(dev->tx_skb[slot]);<br>
> + dev->tx_skb[slot] = NULL;<br>
> + dev->tx_cnt--;<br>
> + ++dev->estats.tx_dropped;<br>
> + }<br>
> +<br>
> dev->tx_skb[slot] = skb;<br>
> dev->tx_desc[slot].data_ptr = dma_map_single(&dev->ofdev->dev,<br>
> skb->data, len,<br>
<br>
</div></div></blockquote></div><br>