Message-ID: <50F7B832.6030307@omicron.at>
Date:	Thu, 17 Jan 2013 09:37:06 +0100
From:	Christian Riesch <christian.riesch@...cron.at>
To:	Mugunthan V N <mugunthanvnm@...com>
CC:	netdev@...r.kernel.org, davem@...emloft.net, s.hauer@...gutronix.de
Subject: Re: [PATCH v4 1/1] net: ethernet: davinci_cpdma: Add boundary for
 rx and tx descriptors

Hi,

On 2013-01-17 01:10, Mugunthan V N wrote:
> When there is heavy transmission traffic in the CPDMA, then Rx descriptors
> memory is also utilized as tx desc memory looses all rx descriptors and the
> driver stops working then.
>
> This patch adds boundary for tx and rx descriptors in bd ram dividing the
> descriptor memory to ensure that during heavy transmission tx doesn't use
> rx descriptors.
>
> This patch is already applied to davinci_emac driver, since CPSW and
> davici_dmac shares the same CPDMA, moving the boundry seperation from
> Davinci EMAC driver to CPDMA driver which was done in the following
> commit
>
> commit 86d8c07ff2448eb4e860e50f34ef6ee78e45c40c
> Author: Sascha Hauer <s.hauer@...gutronix.de>
> Date:   Tue Jan 3 05:27:47 2012 +0000
>
>      net/davinci: do not use all descriptors for tx packets
>
>      The driver uses a shared pool for both rx and tx descriptors.
>      During open it queues fixed number of 128 descriptors for receive
>      packets. For each received packet it tries to queue another
>      descriptor. If this fails the descriptor is lost for rx.
>      The driver has no limitation on tx descriptors to use, so it
>      can happen during a nmap / ping -f attack that the driver
>      allocates all descriptors for tx and looses all rx descriptors.
>      The driver stops working then.
>      To fix this limit the number of tx descriptors used to half of
>      the descriptors available, the rx path uses the other half.
>
>      Tested on a custom board using nmap / ping -f to the board from
>      two different hosts.
>
> Signed-off-by: Mugunthan V N <mugunthanvnm@...com>
> ---
> Changes from v3
> * Changed multiline commenting as per networking style
>
>   drivers/net/ethernet/ti/cpsw.c          |    9 ++++++
>   drivers/net/ethernet/ti/davinci_cpdma.c |   51 ++++++++++++++++++++++++++-----
>   drivers/net/ethernet/ti/davinci_cpdma.h |    1 +
>   drivers/net/ethernet/ti/davinci_emac.c  |   13 ++++----
>   4 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 3772804..b35e6a7 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -374,6 +374,9 @@ void cpsw_tx_handler(void *token, int len, int status)
>   	struct net_device	*ndev = skb->dev;
>   	struct cpsw_priv	*priv = netdev_priv(ndev);
>
> +	/* Check whether the queue is stopped due to stalled tx dma, if the
> +	 * queue is stopped then start the queue as we have free desc for tx
> +	 */
>   	if (unlikely(netif_queue_stopped(ndev)))
>   		netif_start_queue(ndev);
>   	cpts_tx_timestamp(&priv->cpts, skb);
> @@ -736,6 +739,12 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
>   		goto fail;
>   	}
>
> +	/* If there is no more tx desc left free then we need to
> +	 * tell the kernel to stop sending us tx frames.
> +	 */
> +	if (unlikely(cpdma_check_free_tx_desc(priv->txch)))
> +		netif_stop_queue(ndev);
> +
>   	return NETDEV_TX_OK;
>   fail:
>   	priv->stats.tx_dropped++;
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 4995673..4f6d82f 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -105,13 +105,13 @@ struct cpdma_ctlr {
>   };
>
>   struct cpdma_chan {
> +	struct cpdma_desc __iomem	*head, *tail;
> +	void __iomem			*hdp, *cp, *rxfree;
>   	enum cpdma_state		state;
>   	struct cpdma_ctlr		*ctlr;
>   	int				chan_num;
>   	spinlock_t			lock;
> -	struct cpdma_desc __iomem	*head, *tail;
>   	int				count;
> -	void __iomem			*hdp, *cp, *rxfree;

I still do not see any benefit from this rearranging here.

>   	u32				mask;
>   	cpdma_handler_fn		handler;
>   	enum dma_data_direction		dir;
> @@ -217,17 +217,29 @@ desc_from_phys(struct cpdma_desc_pool *pool, dma_addr_t dma)
>   }
>
>   static struct cpdma_desc __iomem *
> -cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc)
> +cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc, bool is_rx)
>   {
>   	unsigned long flags;
>   	int index;
> +	int desc_start;
> +	int desc_end;
>   	struct cpdma_desc __iomem *desc = NULL;
>
>   	spin_lock_irqsave(&pool->lock, flags);
>
> -	index = bitmap_find_next_zero_area(pool->bitmap, pool->num_desc, 0,
> -					   num_desc, 0);
> -	if (index < pool->num_desc) {
> +	if (is_rx) {
> +		desc_start = 0;
> +		desc_end = pool->num_desc/2;
> +		index = bitmap_find_next_zero_area(pool->bitmap,
> +				pool->num_desc/2, 0, num_desc, 0);

Duplicate calls of bitmap_find_next_zero_area, see a few lines below.

> +	 } else {
> +		desc_start = pool->num_desc/2;
> +		desc_end = pool->num_desc;
> +	}
> +
> +	index = bitmap_find_next_zero_area(pool->bitmap,
> +				desc_end, desc_start, num_desc, 0);
> +	if (index < desc_end) {
>   		bitmap_set(pool->bitmap, index, num_desc);
>   		desc = pool->iomap + pool->desc_size * index;
>   		pool->used_desc++;
> @@ -660,6 +672,7 @@ int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
>   	unsigned long			flags;
>   	u32				mode;
>   	int				ret = 0;
> +	bool                            is_rx;
>
>   	spin_lock_irqsave(&chan->lock, flags);
>
> @@ -668,7 +681,8 @@ int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
>   		goto unlock_ret;
>   	}
>
> -	desc = cpdma_desc_alloc(ctlr->pool, 1);
> +	is_rx = (chan->rxfree != 0);

I guess you should use is_rx_chan() here.

> +	desc = cpdma_desc_alloc(ctlr->pool, 1, is_rx);
>   	if (!desc) {
>   		chan->stats.desc_alloc_fail++;
>   		ret = -ENOMEM;
> @@ -704,6 +718,29 @@ unlock_ret:
>   }
>   EXPORT_SYMBOL_GPL(cpdma_chan_submit);
>
> +bool cpdma_check_free_tx_desc(struct cpdma_chan *chan)
> +{
> +	unsigned long flags;
> +	int index;
> +	bool ret;
> +	struct cpdma_ctlr	*ctlr = chan->ctlr;
> +	struct cpdma_desc_pool	*pool = ctlr->pool;
> +
> +	spin_lock_irqsave(&pool->lock, flags);
> +
> +	index = bitmap_find_next_zero_area(pool->bitmap,
> +				pool->num_desc, pool->num_desc/2, 1, 0);

I still wonder if using two separate descriptor pools for rx and tx 
would be a nicer implementation. In this case you could just use 
bitmap_full here, right? And the two pools would have separate 
spinlocks, so rx could not lock tx and vice versa (but as I wrote in an 
earlier email, I am a newbie here, so I don't know if this would be 
beneficial).

Regards, Christian

> +
> +	if (index < pool->num_desc)
> +		ret = true;
> +	else
> +		ret = false;
> +
> +	spin_unlock_irqrestore(&pool->lock, flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cpdma_check_free_tx_desc);
> +
>   static void __cpdma_chan_free(struct cpdma_chan *chan,
>   			      struct cpdma_desc __iomem *desc,
>   			      int outlen, int status)
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
> index afa19a0..8d2aeb2 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.h
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.h
> @@ -88,6 +88,7 @@ int cpdma_chan_process(struct cpdma_chan *chan, int quota);
>   int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable);
>   void cpdma_ctlr_eoi(struct cpdma_ctlr *ctlr);
>   int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable);
> +bool cpdma_check_free_tx_desc(struct cpdma_chan *chan);
>
>   enum cpdma_control {
>   	CPDMA_CMD_IDLE,			/* write-only */
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 8478d98..1c97c81 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -120,7 +120,6 @@ static const char emac_version_string[] = "TI DaVinci EMAC Linux v6.1";
>   #define EMAC_DEF_TX_CH			(0) /* Default 0th channel */
>   #define EMAC_DEF_RX_CH			(0) /* Default 0th channel */
>   #define EMAC_DEF_RX_NUM_DESC		(128)
> -#define EMAC_DEF_TX_NUM_DESC		(128)
>   #define EMAC_DEF_MAX_TX_CH		(1) /* Max TX channels configured */
>   #define EMAC_DEF_MAX_RX_CH		(1) /* Max RX channels configured */
>   #define EMAC_POLL_WEIGHT		(64) /* Default NAPI poll weight */
> @@ -342,7 +341,6 @@ struct emac_priv {
>   	u32 mac_hash2;
>   	u32 multicast_hash_cnt[EMAC_NUM_MULTICAST_BITS];
>   	u32 rx_addr_type;
> -	atomic_t cur_tx;
>   	const char *phy_id;
>   #ifdef CONFIG_OF
>   	struct device_node *phy_node;
> @@ -1050,10 +1048,10 @@ static void emac_tx_handler(void *token, int len, int status)
>   {
>   	struct sk_buff		*skb = token;
>   	struct net_device	*ndev = skb->dev;
> -	struct emac_priv	*priv = netdev_priv(ndev);
> -
> -	atomic_dec(&priv->cur_tx);
>
> +	/* Check whether the queue is stopped due to stalled tx dma, if the
> +	 * queue is stopped then start the queue as we have free desc for tx
> +	 */
>   	if (unlikely(netif_queue_stopped(ndev)))
>   		netif_start_queue(ndev);
>   	ndev->stats.tx_packets++;
> @@ -1101,7 +1099,10 @@ static int emac_dev_xmit(struct sk_buff *skb, struct net_device *ndev)
>   		goto fail_tx;
>   	}
>
> -	if (atomic_inc_return(&priv->cur_tx) >= EMAC_DEF_TX_NUM_DESC)
> +	/* If there is no more tx desc left free then we need to
> +	 * tell the kernel to stop sending us tx frames.
> +	 */
> +	if (unlikely(cpdma_check_free_tx_desc(priv->txch)))
>   		netif_stop_queue(ndev);
>
>   	return NETDEV_TX_OK;
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists