All of lore.kernel.org
 help / color / mirror / Atom feed
From: stigge@antcom.de (Roland Stigge)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn()
Date: Wed, 13 Jun 2012 11:28:07 +0200	[thread overview]
Message-ID: <4FD85D27.5090509@antcom.de> (raw)
In-Reply-To: <1339568172.22704.312.camel@edumazet-glaptop>

On 06/13/2012 08:16 AM, Eric Dumazet wrote:
> On Mon, 2012-06-11 at 21:18 +0200, Eric Dumazet wrote:
>> On Mon, 2012-06-11 at 11:26 +0200, Roland Stigge wrote:
>>
>>> Is it sensible at this point to increase the TX buffers anyway? For
>>> different reasons of course: We have enough SRAM available and TX
>>> buffers (16->32) are still more than RX buffers (48).
>>
>> I doubt it has any impact on performance for a 100Mbit link ?
>>
>> One thing that could be done would be to free skbs in
>> lpc_eth_hard_start_xmit() instead of __lpc_handle_xmit()
>>
> 
> Here is the patch I was thinking about
> 
> (on top of latest net-next)
> 
> Could you please test it ?
>
>  drivers/net/ethernet/nxp/lpc_eth.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
> index 083d671..426f14c 100644
> --- a/drivers/net/ethernet/nxp/lpc_eth.c
> +++ b/drivers/net/ethernet/nxp/lpc_eth.c
> @@ -440,7 +440,7 @@ struct netdata_local {
>  	spinlock_t		lock;
>  	void __iomem		*net_base;
>  	u32			msg_enable;
> -	struct sk_buff		*skb[ENET_TX_DESC];
> +	unsigned int		skblen[ENET_TX_DESC];
>  	unsigned int		last_tx_idx;
>  	unsigned int		num_used_tx_buffs;
>  	struct mii_bus		*mii_bus;
> @@ -908,7 +908,7 @@ static void __lpc_handle_xmit(struct net_device *ndev)
>  
>  	txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
>  	while (pldat->last_tx_idx != txcidx) {
> -		skb = pldat->skb[pldat->last_tx_idx];
> +		unsigned int skblen = pldat->skblen[pldat->last_tx_idx];
>  
>  		/* A buffer is available, get buffer status */
>  		ptxstat = &pldat->tx_stat_v[pldat->last_tx_idx];
> @@ -945,9 +945,8 @@ static void __lpc_handle_xmit(struct net_device *ndev)
>  		} else {
>  			/* Update stats */
>  			ndev->stats.tx_packets++;
> -			ndev->stats.tx_bytes += skb->len;
> +			ndev->stats.tx_bytes += skblen;
>  		}
> -		dev_kfree_skb_irq(skb);
>  
>  		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
>  	}
> @@ -1132,7 +1131,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	memcpy(pldat->tx_buff_v + txidx * ENET_MAXF_SIZE, skb->data, len);
>  
>  	/* Save the buffer and increment the buffer counter */
> -	pldat->skb[txidx] = skb;
> +	pldat->skblen[txidx] = len;
>  	pldat->num_used_tx_buffs++;
>  
>  	/* Start transmit */
> @@ -1147,6 +1146,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	spin_unlock_irq(&pldat->lock);
>  
> +	dev_kfree_skb(skb);
>  	return NETDEV_TX_OK;
>  }

Works fine for a while now.

We can remove the unused variable skb from __lpc_handle_xmit() now,
maybe just do in your patch?

Thanks!

Tested-by: Roland Stigge <stigge@antcom.de>

WARNING: multiple messages have this Message-ID (diff)
From: Roland Stigge <stigge@antcom.de>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	kevin.wells@nxp.com, srinivas.bakki@nxp.com,
	aletes.xgr@gmail.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn()
Date: Wed, 13 Jun 2012 11:28:07 +0200	[thread overview]
Message-ID: <4FD85D27.5090509@antcom.de> (raw)
In-Reply-To: <1339568172.22704.312.camel@edumazet-glaptop>

On 06/13/2012 08:16 AM, Eric Dumazet wrote:
> On Mon, 2012-06-11 at 21:18 +0200, Eric Dumazet wrote:
>> On Mon, 2012-06-11 at 11:26 +0200, Roland Stigge wrote:
>>
>>> Is it sensible at this point to increase the TX buffers anyway? For
>>> different reasons of course: We have enough SRAM available and TX
>>> buffers (16->32) are still more than RX buffers (48).
>>
>> I doubt it has any impact on performance for a 100Mbit link ?
>>
>> One thing that could be done would be to free skbs in
>> lpc_eth_hard_start_xmit() instead of __lpc_handle_xmit()
>>
> 
> Here is the patch I was thinking about
> 
> (on top of latest net-next)
> 
> Could you please test it ?
>
>  drivers/net/ethernet/nxp/lpc_eth.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
> index 083d671..426f14c 100644
> --- a/drivers/net/ethernet/nxp/lpc_eth.c
> +++ b/drivers/net/ethernet/nxp/lpc_eth.c
> @@ -440,7 +440,7 @@ struct netdata_local {
>  	spinlock_t		lock;
>  	void __iomem		*net_base;
>  	u32			msg_enable;
> -	struct sk_buff		*skb[ENET_TX_DESC];
> +	unsigned int		skblen[ENET_TX_DESC];
>  	unsigned int		last_tx_idx;
>  	unsigned int		num_used_tx_buffs;
>  	struct mii_bus		*mii_bus;
> @@ -908,7 +908,7 @@ static void __lpc_handle_xmit(struct net_device *ndev)
>  
>  	txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
>  	while (pldat->last_tx_idx != txcidx) {
> -		skb = pldat->skb[pldat->last_tx_idx];
> +		unsigned int skblen = pldat->skblen[pldat->last_tx_idx];
>  
>  		/* A buffer is available, get buffer status */
>  		ptxstat = &pldat->tx_stat_v[pldat->last_tx_idx];
> @@ -945,9 +945,8 @@ static void __lpc_handle_xmit(struct net_device *ndev)
>  		} else {
>  			/* Update stats */
>  			ndev->stats.tx_packets++;
> -			ndev->stats.tx_bytes += skb->len;
> +			ndev->stats.tx_bytes += skblen;
>  		}
> -		dev_kfree_skb_irq(skb);
>  
>  		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
>  	}
> @@ -1132,7 +1131,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	memcpy(pldat->tx_buff_v + txidx * ENET_MAXF_SIZE, skb->data, len);
>  
>  	/* Save the buffer and increment the buffer counter */
> -	pldat->skb[txidx] = skb;
> +	pldat->skblen[txidx] = len;
>  	pldat->num_used_tx_buffs++;
>  
>  	/* Start transmit */
> @@ -1147,6 +1146,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	spin_unlock_irq(&pldat->lock);
>  
> +	dev_kfree_skb(skb);
>  	return NETDEV_TX_OK;
>  }

Works fine for a while now.

We can remove the unused variable skb from __lpc_handle_xmit() now,
maybe just do in your patch?

Thanks!

Tested-by: Roland Stigge <stigge@antcom.de>

  reply	other threads:[~2012-06-13  9:28 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-06-11  8:03 [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn() Roland Stigge
2012-06-11  8:03 ` Roland Stigge
2012-06-11  8:03 ` [PATCH 2/3] net: lpc_eth: Increase number of TX descriptors Roland Stigge
2012-06-11  8:03   ` Roland Stigge
2012-06-11  8:11   ` David Miller
2012-06-11  8:11     ` David Miller
2012-06-11  8:21   ` Eric Dumazet
2012-06-11  8:21     ` Eric Dumazet
2012-06-11  8:03 ` [PATCH 3/3] net: lpc_eth: Driver cleanup Roland Stigge
2012-06-11  8:03   ` Roland Stigge
2012-06-11  8:10 ` [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn() David Miller
2012-06-11  8:10   ` David Miller
2012-06-11  8:25 ` Eric Dumazet
2012-06-11  8:25   ` Eric Dumazet
2012-06-11  8:36   ` Roland Stigge
2012-06-11  8:36     ` Roland Stigge
2012-06-11  8:53     ` Eric Dumazet
2012-06-11  8:53       ` Eric Dumazet
2012-06-11 17:21       ` [PATCH] net: lpc_eth: fix tx completion Eric Dumazet
2012-06-11 17:21         ` Eric Dumazet
2012-06-11 18:58         ` Roland Stigge
2012-06-11 18:58           ` Roland Stigge
2012-06-11 20:13         ` David Miller
2012-06-11 20:13           ` David Miller
2012-06-11  9:03     ` [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn() David Miller
2012-06-11  9:03       ` David Miller
2012-06-11  9:26       ` Roland Stigge
2012-06-11  9:26         ` Roland Stigge
2012-06-11 19:18         ` Eric Dumazet
2012-06-11 19:18           ` Eric Dumazet
2012-06-13  6:16           ` Eric Dumazet
2012-06-13  6:16             ` Eric Dumazet
2012-06-13  9:28             ` Roland Stigge [this message]
2012-06-13  9:28               ` Roland Stigge
2012-06-13  9:58               ` [PATCH net-next] net: lpc_eth: free skbs in start_xmit Eric Dumazet
2012-06-13  9:58                 ` Eric Dumazet
2012-06-17 23:28                 ` David Miller
2012-06-17 23:28                   ` David Miller
2012-06-11  8:39   ` [PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn() Eric Dumazet
2012-06-11  8:39     ` Eric Dumazet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4FD85D27.5090509@antcom.de \
    --to=stigge@antcom.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.