lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 20 Jun 2008 16:18:35 +0530
From:	Krishna Kumar2 <krkumar2@...ibm.com>
To:	David Miller <davem@...emloft.net>
Cc:	mchan@...adcom.com, netdev@...r.kernel.org,
	vinay@...ux.vnet.ibm.com
Subject: Re: [PATCH 3/3]: tg3: Manage TX backlog in-driver.

Great, and this looks cool for batching too :)

Couple of comments:

1. The modified drivers has a backlog of upto tx_queue_len skbs
    compared to unmodified drivers which had tx_queue_len+q->limit.
    Won't this result in a performance hit since packet drops will
    take place earlier?

2. __tg3_xmit_backlog() should check for not running too long. This
    also means calling netif_schedule() if tx_backlog is !empty, to
    avoid rotting packets in the backlog queue.

Thanks,

- KK

David Miller <davem@...emloft.net> wrote on 06/19/2008 04:40:24 PM:

>
> tg3: Manage TX backlog in-driver.
>
> We no longer stop and wake the generic device queue.
> Instead we manage the backlog inside of the driver,
> and the mid-layer thinks that the device can always
> receive packets.
>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
>  drivers/net/tg3.c |  124
+++++++++++++++++++++++++++++++++--------------------
>  drivers/net/tg3.h |    1 +
>  2 files changed, 78 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 53963da..db960f8 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -3821,11 +3821,65 @@ static void tg3_tx_recover(struct tg3 *tp)
>     spin_unlock(&tp->lock);
>  }
>
> +static inline u32 __tg3_tx_avail(struct tg3 *tp, u32 prod)
> +{
> +   return (tp->tx_pending -
> +      ((prod - tp->tx_cons) & (TG3_TX_RING_SIZE - 1)));
> +}
> +
>  static inline u32 tg3_tx_avail(struct tg3 *tp)
>  {
>     smp_mb();
> -   return (tp->tx_pending -
> -      ((tp->tx_prod - tp->tx_cons) & (TG3_TX_RING_SIZE - 1)));
> +   return __tg3_tx_avail(tp, tp->tx_prod);
> +}
> +
> +/* Return true if the TX ring has enough space to queue SKB.  */
> +static bool __tg3_tx_has_space(struct tg3 *tp, u32 prod, struct sk_buff
*skb)
> +{
> +   int chunks = skb_shinfo(skb)->nr_frags + 1;
> +   if (__tg3_tx_avail(tp, prod) <= chunks)
> +      return false;
> +   return true;
> +}
> +
> +static bool tg3_tx_has_space(struct tg3 *tp, struct sk_buff *skb)
> +{
> +   return __tg3_tx_has_space(tp, tp->tx_prod, skb);
> +}
> +
> +/* Tell the chip that N is the current TX producer index.  */
> +static void __tg3_tx_hit(struct tg3 *tp, u32 n)
> +{
> +   if (unlikely(n == tp->tx_prod))
> +      return;
> +   tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), n);
> +   tp->tx_prod = n;
> +   mmiowb();
> +   tp->dev->trans_start = jiffies;
> +}
> +
> +static u32 __tg3_xmit_one(struct tg3 *tp, struct net_device *dev,
> +           struct sk_buff *skb, u32 tx_entry);
> +
> +/* Queue as many backlogged TX packets as possible.  Invoked
> + * under tx_lock.
> + */
> +static void __tg3_xmit_backlog(struct tg3 *tp)
> +{
> +   struct sk_buff *skb;
> +   u32 entry;
> +
> +   if (unlikely(skb_queue_empty(&tp->tx_backlog)))
> +      return;
> +
> +   entry = tp->tx_prod;
> +   while ((skb = skb_peek(&tp->tx_backlog)) != NULL) {
> +      if (!__tg3_tx_has_space(tp, entry, skb))
> +         break;
> +      __skb_unlink(skb, &tp->tx_backlog);
> +      entry = __tg3_xmit_one(tp, tp->dev, skb, entry);
> +   }
> +   __tg3_tx_hit(tp, entry);
>  }
>
>  /* Tigon3 never reports partial packet sends.  So we do not
> @@ -3880,18 +3934,16 @@ static void tg3_tx(struct tg3 *tp)
>     tp->tx_cons = sw_idx;
>
>     /* Need to make the tx_cons update visible to tg3_start_xmit()
> -    * before checking for netif_queue_stopped().  Without the
> -    * memory barrier, there is a small possibility that tg3_start_xmit()
> +    * before checking the TX backlog.  Without the memory
> +    * barrier, there is a small possibility that tg3_start_xmit()
>      * will miss it and cause the queue to be stopped forever.
>      */
>     smp_mb();
>
> -   if (unlikely(netif_queue_stopped(tp->dev) &&
> -           (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp)))) {
> +   if (!skb_queue_empty(&tp->tx_backlog) &&
> +       (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp))) {
>        netif_tx_lock(tp->dev);
> -      if (netif_queue_stopped(tp->dev) &&
> -          (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp)))
> -         netif_wake_queue(tp->dev);
> +      __tg3_xmit_backlog(tp);
>        netif_tx_unlock(tp->dev);
>     }
>  }
> @@ -4685,9 +4737,6 @@ static void tg3_set_txd(struct tg3 *tp, int entry,
>     txd->vlan_tag = vlan_tag << TXD_VLAN_TAG_SHIFT;
>  }
>
> -static u32 __tg3_xmit_one(struct tg3 *tp, struct net_device *dev,
> -           struct sk_buff *skb, u32 tx_entry);
> -
>  /* Use GSO to workaround a rare TSO bug that may be triggered when the
>   * TSO header is greater than 80 bytes.
>   */
> @@ -4868,44 +4917,22 @@ out:
>     return entry;
>  }
>
> +static void __tg3_tx_queue_backlog(struct tg3 *tp, struct sk_buff *skb)
> +{
> +   if (skb_queue_len(&tp->tx_backlog) < tp->dev->tx_queue_len)
> +      __skb_queue_tail(&tp->tx_backlog, skb);
> +   else
> +      dev_kfree_skb(skb);
> +}
> +
>  static int tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>     struct tg3 *tp = netdev_priv(dev);
> -   u32 entry;
>
> -   /* We are running in BH disabled context with netif_tx_lock
> -    * and TX reclaim runs via tp->napi.poll inside of a software
> -    * interrupt.  Furthermore, IRQ processing runs lockless so we have
> -    * no IRQ context deadlocks to worry about either.  Rejoice!
> -    */
> -   if (unlikely(tg3_tx_avail(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
> -      if (!netif_queue_stopped(dev)) {
> -         netif_stop_queue(dev);
> -
> -         /* This is a hard error, log it. */
> -         printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
> -                "queue awake!\n", dev->name);
> -      }
> -      return NETDEV_TX_BUSY;
> -   }
> -
> -   entry = __tg3_xmit_one(tp, dev, skb, tp->tx_prod);
> -
> -   if (entry != tp->tx_prod) {
> -      /* Packets are ready, update Tx producer idx local
> -       * and on card.
> -       */
> -      tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW),
> -              entry);
> -
> -      tp->tx_prod = entry;
> -      if (unlikely(tg3_tx_avail(tp) <= (MAX_SKB_FRAGS + 1))) {
> -         netif_stop_queue(dev);
> -         if (tg3_tx_avail(tp) > TG3_TX_WAKEUP_THRESH(tp))
> -            netif_wake_queue(tp->dev);
> -      }
> -      mmiowb();
> -      dev->trans_start = jiffies;
> +   if (unlikely(!tg3_tx_has_space(tp, skb))) {
> +      __tg3_tx_queue_backlog(tp, skb);
> +   } else {
> +      __tg3_tx_hit(tp, __tg3_xmit_one(tp, dev, skb, tp->tx_prod));
>     }
>
>     return NETDEV_TX_OK;
> @@ -4979,6 +5006,7 @@ static int tg3_change_mtu(struct net_device *dev,
int new_mtu)
>  static void tg3_free_rings(struct tg3 *tp)
>  {
>     struct ring_info *rxp;
> +   struct sk_buff *skb;
>     int i;
>
>     for (i = 0; i < TG3_RX_RING_SIZE; i++) {
> @@ -5009,7 +5037,6 @@ static void tg3_free_rings(struct tg3 *tp)
>
>     for (i = 0; i < TG3_TX_RING_SIZE; ) {
>        struct tx_ring_info *txp;
> -      struct sk_buff *skb;
>        int j;
>
>        txp = &tp->tx_buffers[i];
> @@ -5039,6 +5066,8 @@ static void tg3_free_rings(struct tg3 *tp)
>
>        dev_kfree_skb_any(skb);
>     }
> +   while ((skb = __skb_dequeue(&tp->tx_backlog)) != NULL)
> +      dev_kfree_skb_any(skb);
>  }
>
>  /* Initialize tx/rx rings for packet processing.
> @@ -13228,6 +13257,7 @@ static int __devinit tg3_init_one(struct pci_dev
*pdev,
>     spin_lock_init(&tp->lock);
>     spin_lock_init(&tp->indirect_lock);
>     INIT_WORK(&tp->reset_task, tg3_reset_task);
> +   skb_queue_head_init(&tp->tx_backlog);
>
>     tp->regs = ioremap_nocache(tg3reg_base, tg3reg_len);
>     if (!tp->regs) {
> diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
> index c89ca40..d396c87 100644
> --- a/drivers/net/tg3.h
> +++ b/drivers/net/tg3.h
> @@ -2391,6 +2391,7 @@ struct tg3 {
>     u32            tx_prod;
>     u32            tx_cons;
>     u32            tx_pending;
> +   struct sk_buff_head      tx_backlog;
>
>     struct tg3_tx_buffer_desc   *tx_ring;
>     struct tx_ring_info      *tx_buffers;
> --
> 1.5.6.rc3.21.g8c6b5
>

--
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