| 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: <CAGVrzcaHLa1kT2wZc993cZFX2geQ4JgOO-u6JcWHFKcn-OZhZw@mail.gmail.com>
Date: Tue, 3 Sep 2013 19:59:42 +0100
From: Florian Fainelli <f.fainelli@...il.com>
To: Claudiu Manoil <claudiu.manoil@...escale.com>
Cc: netdev <netdev@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH][net-next] gianfar: Fix reported number of sent bytes to BQL
Hello Claudiu,
2013/8/30 Claudiu Manoil <claudiu.manoil@...escale.com>:
> Fix the amount of sent bytes reported to BQL by reporting the
> number of bytes on wire in the xmit routine, and recording that
> value for each skb in order to be correctly confirmed on Tx
> confirmation cleanup.
>
> Reporting skb->len to BQL just before exiting xmit is not correct
> due to possible insertions of TOE block and alignment bytes in the
> skb->data, which are being stripped off by the controller before
> transmission on wire. This led to mismatch of (incorrectly)
> reported bytes to BQL b/w xmit and Tx confirmation, resulting in
> Tx timeout firing, for the h/w tx timestamping acceleration case.
>
> There's no easy way to obtain the number of bytes on wire in the Tx
> confirmation routine, so skb->cb is used to convey that information
> from xmit to Tx confirmation, for now (as proposed by Eric). Revived
> the currently unused GFAR_CB() construct for that purpose.
I do not see much difference between what this patch does and what the
current net-next drivers does. If you need to correctly account for
this, it seems to me like you should move the stats/bytes_sent
computation below this line:
if (unlikely(do_tstamp)) {
skb_push(skb, GMAC_TXPAL_LEN);
memset(skb->data, 0, GMAC_TXPAL_LEN);
}
to account for the SKB length update?
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@...escale.com>
>
> Cc: Eric Dumazet <eric.dumazet@...il.com>
> ---
> drivers/net/ethernet/freescale/gianfar.c | 18 +++++++++++-------
> drivers/net/ethernet/freescale/gianfar.h | 2 +-
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index b2c91dc..c4eaade 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2092,7 +2092,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> int do_tstamp, do_csum, do_vlan;
> u32 bufaddr;
> unsigned long flags;
> - unsigned int nr_frags, nr_txbds, length, fcb_len = 0;
> + unsigned int nr_frags, nr_txbds, bytes_sent, fcb_len = 0;
>
> rq = skb->queue_mapping;
> tx_queue = priv->tx_queue[rq];
> @@ -2147,7 +2147,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> }
>
> /* Update transmit stats */
> - tx_queue->stats.tx_bytes += skb->len;
> + bytes_sent = skb->len;
> + tx_queue->stats.tx_bytes += bytes_sent;
> + /* keep Tx bytes on wire for BQL accounting */
> + GFAR_CB(skb)->bytes_sent = bytes_sent;
> tx_queue->stats.tx_packets++;
>
> txbdp = txbdp_start = tx_queue->cur_tx;
> @@ -2167,12 +2170,13 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> } else {
> /* Place the fragment addresses and lengths into the TxBDs */
> for (i = 0; i < nr_frags; i++) {
> + unsigned int frag_len;
> /* Point at the next BD, wrapping as needed */
> txbdp = next_txbd(txbdp, base, tx_queue->tx_ring_size);
>
> - length = skb_shinfo(skb)->frags[i].size;
> + frag_len = skb_shinfo(skb)->frags[i].size;
>
> - lstatus = txbdp->lstatus | length |
> + lstatus = txbdp->lstatus | frag_len |
> BD_LFLAG(TXBD_READY);
>
> /* Handle the last BD specially */
> @@ -2182,7 +2186,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> bufaddr = skb_frag_dma_map(priv->dev,
> &skb_shinfo(skb)->frags[i],
> 0,
> - length,
> + frag_len,
> DMA_TO_DEVICE);
>
> /* set the TxBD length and buffer pointer */
> @@ -2250,7 +2254,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
> lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
> }
>
> - netdev_tx_sent_queue(txq, skb->len);
> + netdev_tx_sent_queue(txq, bytes_sent);
>
> /* We can work in parallel with gfar_clean_tx_ring(), except
> * when modifying num_txbdfree. Note that we didn't grab the lock
> @@ -2570,7 +2574,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
> bdp = next_txbd(bdp, base, tx_ring_size);
> }
>
> - bytes_sent += skb->len;
> + bytes_sent += GFAR_CB(skb)->bytes_sent;
>
> dev_kfree_skb_any(skb);
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
> index 46f56f3..04112b9 100644
> --- a/drivers/net/ethernet/freescale/gianfar.h
> +++ b/drivers/net/ethernet/freescale/gianfar.h
> @@ -575,7 +575,7 @@ struct rxfcb {
> };
>
> struct gianfar_skb_cb {
> - int alignamount;
> + unsigned int bytes_sent; /* bytes-on-wire (i.e. no FCB) */
> };
>
> #define GFAR_CB(skb) ((struct gianfar_skb_cb *)((skb)->cb))
> --
> 1.7.11.7
>
>
> --
> 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
--
Florian
--
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