| 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 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <OFD7CC7687.E0D15399-ON6525746E.0038A920-6525746E.003B6162@in.ibm.com>
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