diff mbox

[1/1] net: ethernet: davinci_cpdma: Add boundary for rx and tx descriptors

Message ID 1355125052-28448-1-git-send-email-mugunthanvnm@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mugunthan V N Dec. 10, 2012, 7:37 a.m. UTC
When there is heavy transmission traffic in the CPDMA, then Rx descriptors
memory is also utilized as tx desc memory this leads to reduced rx desc memory
which leads to poor performance.

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 uses 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@pengutronix.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@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c |   20 ++++++++++++++------
 drivers/net/ethernet/ti/davinci_emac.c  |    8 --------
 2 files changed, 14 insertions(+), 14 deletions(-)

Comments

Christian Riesch Dec. 10, 2012, 8:11 a.m. UTC | #1
Hi,

On Mon, Dec 10, 2012 at 8:37 AM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> When there is heavy transmission traffic in the CPDMA, then Rx descriptors
> memory is also utilized as tx desc memory this leads to reduced rx desc memory
> which leads to poor performance.
>
> 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 uses 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@pengutronix.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@ti.com>
> ---
>  drivers/net/ethernet/ti/davinci_cpdma.c |   20 ++++++++++++++------
>  drivers/net/ethernet/ti/davinci_emac.c  |    8 --------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 4995673..d37f546 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;

Why?

>         u32                             mask;
>         cpdma_handler_fn                handler;
>         enum dma_data_direction         dir;
> @@ -217,7 +217,7 @@ 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;
> @@ -225,8 +225,14 @@ cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc)
>
>         spin_lock_irqsave(&pool->lock, flags);
>
> -       index = bitmap_find_next_zero_area(pool->bitmap, pool->num_desc, 0,
> -                                          num_desc, 0);
> +       if (is_rx) {
> +               index = bitmap_find_next_zero_area(pool->bitmap,
> +                               pool->num_desc/2, 0, num_desc, 0);
> +        } else {
> +               index = bitmap_find_next_zero_area(pool->bitmap,
> +                               pool->num_desc, pool->num_desc/2, num_desc, 0);
> +       }

Would it make sense to use two separate pools for rx and tx instead,
struct cpdma_desc_pool *rxpool, *txpool? It would result in using
separate spinlocks for rx and tx, could this be an advantage? (I am a
newbie in this field...)

> +
>         if (index < pool->num_desc) {
>                 bitmap_set(pool->bitmap, index, num_desc);
>                 desc = pool->iomap + pool->desc_size * index;
> @@ -660,6 +666,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 +675,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);
> +       desc = cpdma_desc_alloc(ctlr->pool, 1, is_rx);
>         if (!desc) {
>                 chan->stats.desc_alloc_fail++;
>                 ret = -ENOMEM;
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index fce89a0..f349273 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,9 +1048,6 @@ 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);
>
>         if (unlikely(netif_queue_stopped(ndev)))
>                 netif_start_queue(ndev);
> @@ -1101,9 +1096,6 @@ 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)
> -               netif_stop_queue(ndev);
> -
>         return NETDEV_TX_OK;
>
>  fail_tx:
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christian Riesch Dec. 10, 2012, 8:24 a.m. UTC | #2
Hi again,

On Mon, Dec 10, 2012 at 8:37 AM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> When there is heavy transmission traffic in the CPDMA, then Rx descriptors
> memory is also utilized as tx desc memory this leads to reduced rx desc memory
> which leads to poor performance.
>

"poor performance" is an understatement, see Sascha's description of
his patch. At initialization of the driver, half of the descriptors in
the pool are allocated for rx. When a packet arrives, one of the rx
descriptors is released and a new one is allocated. If tx allocates
this descriptor in the meantime, it is lost for rx forever! If tx
consumes all rx descriptors this way, the rx channel is dead!

Regards, Christian

> 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 uses 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@pengutronix.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@ti.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mugunthan V N Dec. 10, 2012, 11:28 a.m. UTC | #3
On 12/10/2012 1:41 PM, Christian Riesch wrote:
> Hi,
>
> On Mon, Dec 10, 2012 at 8:37 AM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> When there is heavy transmission traffic in the CPDMA, then Rx descriptors
>> memory is also utilized as tx desc memory this leads to reduced rx desc memory
>> which leads to poor performance.
>>
>> 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 uses 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@pengutronix.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@ti.com>
>> ---
>>   drivers/net/ethernet/ti/davinci_cpdma.c |   20 ++++++++++++++------
>>   drivers/net/ethernet/ti/davinci_emac.c  |    8 --------
>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index 4995673..d37f546 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;
> Why?
Its just a code clean-up to have iomem variables at one place.
>
>>          u32                             mask;
>>          cpdma_handler_fn                handler;
>>          enum dma_data_direction         dir;
>> @@ -217,7 +217,7 @@ 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;
>> @@ -225,8 +225,14 @@ cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc)
>>
>>          spin_lock_irqsave(&pool->lock, flags);
>>
>> -       index = bitmap_find_next_zero_area(pool->bitmap, pool->num_desc, 0,
>> -                                          num_desc, 0);
>> +       if (is_rx) {
>> +               index = bitmap_find_next_zero_area(pool->bitmap,
>> +                               pool->num_desc/2, 0, num_desc, 0);
>> +        } else {
>> +               index = bitmap_find_next_zero_area(pool->bitmap,
>> +                               pool->num_desc, pool->num_desc/2, num_desc, 0);
>> +       }
> Would it make sense to use two separate pools for rx and tx instead,
> struct cpdma_desc_pool *rxpool, *txpool? It would result in using
> separate spinlocks for rx and tx, could this be an advantage? (I am a
> newbie in this field...)
I don't think separating pool will give an advantage, the same is 
achieved by separating
the pool into first half and last half.

Regards
Mugunthan V N
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mugunthan V N Dec. 10, 2012, 11:29 a.m. UTC | #4
On 12/10/2012 1:54 PM, Christian Riesch wrote:
> Hi again,
>
> On Mon, Dec 10, 2012 at 8:37 AM, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> When there is heavy transmission traffic in the CPDMA, then Rx descriptors
>> memory is also utilized as tx desc memory this leads to reduced rx desc memory
>> which leads to poor performance.
>>
> "poor performance" is an understatement, see Sascha's description of
> his patch. At initialization of the driver, half of the descriptors in
> the pool are allocated for rx. When a packet arrives, one of the rx
> descriptors is released and a new one is allocated. If tx allocates
> this descriptor in the meantime, it is lost for rx forever! If tx
> consumes all rx descriptors this way, the rx channel is dead!
>
> Regards, Christian
>
>> 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 uses 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@pengutronix.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@ti.com>
Will change the commit description and resubmit the patch.

Regards
Mugunthan V N
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 4995673..d37f546 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;
 	u32				mask;
 	cpdma_handler_fn		handler;
 	enum dma_data_direction		dir;
@@ -217,7 +217,7 @@  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;
@@ -225,8 +225,14 @@  cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc)
 
 	spin_lock_irqsave(&pool->lock, flags);
 
-	index = bitmap_find_next_zero_area(pool->bitmap, pool->num_desc, 0,
-					   num_desc, 0);
+	if (is_rx) {
+		index = bitmap_find_next_zero_area(pool->bitmap,
+				pool->num_desc/2, 0, num_desc, 0);
+	 } else {
+		index = bitmap_find_next_zero_area(pool->bitmap,
+				pool->num_desc, pool->num_desc/2, num_desc, 0);
+	}
+
 	if (index < pool->num_desc) {
 		bitmap_set(pool->bitmap, index, num_desc);
 		desc = pool->iomap + pool->desc_size * index;
@@ -660,6 +666,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 +675,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);
+	desc = cpdma_desc_alloc(ctlr->pool, 1, is_rx);
 	if (!desc) {
 		chan->stats.desc_alloc_fail++;
 		ret = -ENOMEM;
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index fce89a0..f349273 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,9 +1048,6 @@  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);
 
 	if (unlikely(netif_queue_stopped(ndev)))
 		netif_start_queue(ndev);
@@ -1101,9 +1096,6 @@  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)
-		netif_stop_queue(ndev);
-
 	return NETDEV_TX_OK;
 
 fail_tx: