* [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path
@ 2013-02-14 15:00 Claudiu Manoil
2013-02-14 15:00 ` [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private Claudiu Manoil
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw)
To: netdev; +Cc: Paul Gortmaker, David S. Miller
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>,
"David S. Miller" <davem@davemloft.net>
Hi Dave,
These patches went already through a review round with Paul.
Changes since the initial version of the patches:
* split the 1st patch into two patches (01 & 02, as suggested by Paul)
* added the suggested trivial cleanup patch:
gianfar: gfar_process_frame returns void
* provided "long logs" with detailed comments and explanations
Thank you.
Regards,
Claudiu
Claudiu Manoil (7):
gianfar: Remove unused device_node ref in gfar_private
gianfar: Add device ref (dev) in gfar_private
gianfar: Cleanup and optimize struct gfar_private
gianfar: GRO_DROP is unlikely
gianfar: gfar_process_frame returns void
gianfar: Remove wrong buffer size conditioning to VLAN h/w offload
gianfar: Fix and cleanup Rx FCB indication
drivers/net/ethernet/freescale/gianfar.c | 85 ++++++++++++---------------
drivers/net/ethernet/freescale/gianfar.h | 96 +++++++++++++++--------------
2 files changed, 88 insertions(+), 93 deletions(-)
--
1.7.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private 2013-02-14 15:00 [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 2/7] gianfar: Add device ref (dev) " Claudiu Manoil 2013-02-14 16:49 ` [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Paul Gortmaker 2013-02-14 18:32 ` David Miller 2 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller Remove unused device node pointer. Remove duplicated SET_NETDEV_DEV(). Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 4 ---- drivers/net/ethernet/freescale/gianfar.h | 1 - 2 files changed, 0 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index c82f677..2c6b569 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -663,7 +663,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) return -ENOMEM; priv = netdev_priv(dev); - priv->node = ofdev->dev.of_node; priv->ndev = dev; priv->num_tx_queues = num_tx_qs; @@ -1001,7 +1000,6 @@ static int gfar_probe(struct platform_device *ofdev) priv = netdev_priv(dev); priv->ndev = dev; priv->ofdev = ofdev; - priv->node = ofdev->dev.of_node; SET_NETDEV_DEV(dev, &ofdev->dev); spin_lock_init(&priv->bflock); @@ -1038,8 +1036,6 @@ static int gfar_probe(struct platform_device *ofdev) /* Set the dev->base_addr to the gfar reg region */ dev->base_addr = (unsigned long) regs; - SET_NETDEV_DEV(dev, &ofdev->dev); - /* Fill in the dev structure */ dev->watchdog_timeo = TX_TIMEOUT; dev->mtu = 1500; diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 78125f1..8b4de57 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1060,7 +1060,6 @@ struct gfar_private { unsigned int total_tx_ring_size; unsigned int total_rx_ring_size; - struct device_node *node; struct net_device *ndev; struct platform_device *ofdev; enum gfar_errata errata; -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 2/7] gianfar: Add device ref (dev) in gfar_private 2013-02-14 15:00 ` [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 3/7] gianfar: Cleanup and optimize struct gfar_private Claudiu Manoil 0 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller Use device pointer (dev) to simplify the code and to avoid double indirections, especially on the hot path. Basically, instead of accessing priv to get the ofdev reference and then accessing the ofdev structure to dereference the needed dev pointer, we will get the dev pointer directly from priv. The dev pointer is required on the hot path, see gfar_new_rxbdp or gfar_clean_rx_ring (or xmit), and this patch makes it available directly from priv's 1st cacheline. This change is reflected at asm level too, taking (the hot) gfar_new_rxbdp(): initial version - 18c0: 7c 7e 1b 78 mr r30,r3 18d0: 81 69 04 3c lwz r11,1084(r9) 18d8: 34 6b 00 10 addic. r3,r11,16 18dc: 41 82 00 08 beq- 18e4 patched version - 18d0: 80 69 04 38 lwz r3,1080(r9) 18d8: 2f 83 00 00 cmpwi cr7,r3,0 18dc: 41 9e 00 08 beq- cr7,18e4 Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 25 +++++++++++++------------ drivers/net/ethernet/freescale/gianfar.h | 1 + 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 2c6b569..592d297 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -231,7 +231,7 @@ static int gfar_alloc_skb_resources(struct net_device *ndev) dma_addr_t addr; int i, j, k; struct gfar_private *priv = netdev_priv(ndev); - struct device *dev = &priv->ofdev->dev; + struct device *dev = priv->dev; struct gfar_priv_tx_q *tx_queue = NULL; struct gfar_priv_rx_q *rx_queue = NULL; @@ -1000,6 +1000,7 @@ static int gfar_probe(struct platform_device *ofdev) priv = netdev_priv(dev); priv->ndev = dev; priv->ofdev = ofdev; + priv->dev = &ofdev->dev; SET_NETDEV_DEV(dev, &ofdev->dev); spin_lock_init(&priv->bflock); @@ -1713,13 +1714,13 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue) if (!tx_queue->tx_skbuff[i]) continue; - dma_unmap_single(&priv->ofdev->dev, txbdp->bufPtr, + dma_unmap_single(priv->dev, txbdp->bufPtr, txbdp->length, DMA_TO_DEVICE); txbdp->lstatus = 0; for (j = 0; j < skb_shinfo(tx_queue->tx_skbuff[i])->nr_frags; j++) { txbdp++; - dma_unmap_page(&priv->ofdev->dev, txbdp->bufPtr, + dma_unmap_page(priv->dev, txbdp->bufPtr, txbdp->length, DMA_TO_DEVICE); } txbdp++; @@ -1740,8 +1741,8 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue) for (i = 0; i < rx_queue->rx_ring_size; i++) { if (rx_queue->rx_skbuff[i]) { - dma_unmap_single(&priv->ofdev->dev, - rxbdp->bufPtr, priv->rx_buffer_size, + dma_unmap_single(priv->dev, rxbdp->bufPtr, + priv->rx_buffer_size, DMA_FROM_DEVICE); dev_kfree_skb_any(rx_queue->rx_skbuff[i]); rx_queue->rx_skbuff[i] = NULL; @@ -1780,7 +1781,7 @@ static void free_skb_resources(struct gfar_private *priv) free_skb_rx_queue(rx_queue); } - dma_free_coherent(&priv->ofdev->dev, + dma_free_coherent(priv->dev, sizeof(struct txbd8) * priv->total_tx_ring_size + sizeof(struct rxbd8) * priv->total_rx_ring_size, priv->tx_queue[0]->tx_bd_base, @@ -2160,7 +2161,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) if (i == nr_frags - 1) lstatus |= BD_LFLAG(TXBD_LAST | TXBD_INTERRUPT); - bufaddr = skb_frag_dma_map(&priv->ofdev->dev, + bufaddr = skb_frag_dma_map(priv->dev, &skb_shinfo(skb)->frags[i], 0, length, @@ -2212,7 +2213,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) lstatus |= BD_LFLAG(TXBD_TOE); } - txbdp_start->bufPtr = dma_map_single(&priv->ofdev->dev, skb->data, + txbdp_start->bufPtr = dma_map_single(priv->dev, skb->data, skb_headlen(skb), DMA_TO_DEVICE); /* If time stamping is requested one additional TxBD must be set up. The @@ -2525,7 +2526,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) } else buflen = bdp->length; - dma_unmap_single(&priv->ofdev->dev, bdp->bufPtr, + dma_unmap_single(priv->dev, bdp->bufPtr, buflen, DMA_TO_DEVICE); if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) { @@ -2544,7 +2545,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) bdp = next_txbd(bdp, base, tx_ring_size); for (i = 0; i < frags; i++) { - dma_unmap_page(&priv->ofdev->dev, bdp->bufPtr, + dma_unmap_page(priv->dev, bdp->bufPtr, bdp->length, DMA_TO_DEVICE); bdp->lstatus &= BD_LFLAG(TXBD_WRAP); bdp = next_txbd(bdp, base, tx_ring_size); @@ -2610,7 +2611,7 @@ static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp, struct gfar_private *priv = netdev_priv(dev); dma_addr_t buf; - buf = dma_map_single(&priv->ofdev->dev, skb->data, + buf = dma_map_single(priv->dev, skb->data, priv->rx_buffer_size, DMA_FROM_DEVICE); gfar_init_rxbdp(rx_queue, bdp, buf); } @@ -2775,7 +2776,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit) skb = rx_queue->rx_skbuff[rx_queue->skb_currx]; - dma_unmap_single(&priv->ofdev->dev, bdp->bufPtr, + dma_unmap_single(priv->dev, bdp->bufPtr, priv->rx_buffer_size, DMA_FROM_DEVICE); if (unlikely(!(bdp->status & RXBD_ERR) && diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 8b4de57..8b27e5f 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1060,6 +1060,7 @@ struct gfar_private { unsigned int total_tx_ring_size; unsigned int total_rx_ring_size; + struct device *dev; struct net_device *ndev; struct platform_device *ofdev; enum gfar_errata errata; -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 3/7] gianfar: Cleanup and optimize struct gfar_private 2013-02-14 15:00 ` [PATCH net-next 2/7] gianfar: Add device ref (dev) " Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 4/7] gianfar: GRO_DROP is unlikely Claudiu Manoil 0 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller Group run-time critical fields within the 1st cacheline (32B) followed by the tx|rx_queue reference arrays and the interrupt group instances (gfargrp), all cacheline aligned. This has several benefits. Firstly comes the performance benefit by having the members required by the driver's hot path re-grouped in the structure's first cache lines, whereas the unimportant members were pushed towards the end of the struct. Another benefit comes from eliminating a 24 byte memory hole that was rendering gfar_priv's 2nd cacheline useless. The default gcc layout of gfar_private leaves an implicit 24 byte hole after the errata (enum) member. This patch fixes it. The uchar bitfields were pushed towards the end of the struct as these are not run-time performance critical (used for init time operations). Because there is no other 2 byte member around to couple the uchar bitfields memeber with, we will have an addititnal 2 byte hole after the bitfields. This is unsignificant however, and it doesn't influence gfar_priv's size, because the whole structure is padded to be a 32B multiple. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.h | 93 +++++++++++++++-------------- 1 files changed, 48 insertions(+), 45 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 8b27e5f..1e2ce8b 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1049,28 +1049,64 @@ enum gfar_errata { * the buffer descriptor determines the actual condition. */ struct gfar_private { - - /* Indicates how many tx, rx queues are enabled */ - unsigned int num_tx_queues; unsigned int num_rx_queues; - unsigned int num_grps; - unsigned int mode; - - /* The total tx and rx ring size for the enabled queues */ - unsigned int total_tx_ring_size; - unsigned int total_rx_ring_size; struct device *dev; struct net_device *ndev; - struct platform_device *ofdev; enum gfar_errata errata; + unsigned int rx_buffer_size; + + u16 padding; + + /* HW time stamping enabled flag */ + int hwts_rx_en; + int hwts_tx_en; - struct gfar_priv_grp gfargrp[MAXGROUPS]; struct gfar_priv_tx_q *tx_queue[MAX_TX_QS]; struct gfar_priv_rx_q *rx_queue[MAX_RX_QS]; + struct gfar_priv_grp gfargrp[MAXGROUPS]; + + u32 device_flags; + + unsigned int mode; + unsigned int num_tx_queues; + unsigned int num_grps; + + /* Network Statistics */ + struct gfar_extra_stats extra_stats; + + /* PHY stuff */ + phy_interface_t interface; + struct device_node *phy_node; + struct device_node *tbi_node; + struct phy_device *phydev; + struct mii_bus *mii_bus; + int oldspeed; + int oldduplex; + int oldlink; + + /* Bitfield update lock */ + spinlock_t bflock; + + uint32_t msg_enable; + + struct work_struct reset_task; + + struct platform_device *ofdev; + unsigned char + extended_hash:1, + bd_stash_en:1, + rx_filer_enable:1, + /* Wake-on-LAN enabled */ + wol_en:1, + /* Enable priorty based Tx scheduling in Hw */ + prio_sched_en:1; + + /* The total tx and rx ring size for the enabled queues */ + unsigned int total_tx_ring_size; + unsigned int total_rx_ring_size; /* RX per device parameters */ - unsigned int rx_buffer_size; unsigned int rx_stash_size; unsigned int rx_stash_index; @@ -1089,39 +1125,6 @@ struct gfar_private { unsigned int fifo_starve; unsigned int fifo_starve_off; - /* Bitfield update lock */ - spinlock_t bflock; - - phy_interface_t interface; - struct device_node *phy_node; - struct device_node *tbi_node; - u32 device_flags; - unsigned char - extended_hash:1, - bd_stash_en:1, - rx_filer_enable:1, - wol_en:1, /* Wake-on-LAN enabled */ - prio_sched_en:1; /* Enable priorty based Tx scheduling in Hw */ - unsigned short padding; - - /* PHY stuff */ - struct phy_device *phydev; - struct mii_bus *mii_bus; - int oldspeed; - int oldduplex; - int oldlink; - - uint32_t msg_enable; - - struct work_struct reset_task; - - /* Network Statistics */ - struct gfar_extra_stats extra_stats; - - /* HW time stamping enabled flag */ - int hwts_rx_en; - int hwts_tx_en; - /*Filer table*/ unsigned int ftp_rqfpr[MAX_FILER_IDX + 1]; unsigned int ftp_rqfcr[MAX_FILER_IDX + 1]; -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 4/7] gianfar: GRO_DROP is unlikely 2013-02-14 15:00 ` [PATCH net-next 3/7] gianfar: Cleanup and optimize struct gfar_private Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 5/7] gianfar: gfar_process_frame returns void Claudiu Manoil 0 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller The change is significant since it affects the rx hot path. Paul observed and documented the effects at asm level, see below: "It turns out that it does make a difference, since gfar_process_frame gets inlined, and so the increment code gets moved out of line (I have marked the if statment with * and the increment code within "-----"): ------------------------- as is currently ------------------ 4d14: 80 61 00 18 lwz r3,24(r1) 4d18: 7f c4 f3 78 mr r4,r30 4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c> * 4d20: 2f 83 00 04 cmpwi cr7,r3,4 4d24: 40 9e 00 1c bne- cr7,4d40 <gfar_clean_rx_ring+0x130> ---------------------------- 4d28: 81 3c 01 f8 lwz r9,504(r28) 4d2c: 81 5c 01 fc lwz r10,508(r28) 4d30: 31 4a 00 01 addic r10,r10,1 4d34: 7d 29 01 94 addze r9,r9 4d38: 91 3c 01 f8 stw r9,504(r28) 4d3c: 91 5c 01 fc stw r10,508(r28) ---------------------------- 4d40: a0 1f 00 24 lhz r0,36(r31) 4d44: 81 3f 00 00 lwz r9,0(r31) 4d48: 7f a4 eb 78 mr r4,r29 4d4c: 7f e3 fb 78 mr r3,r31 -------------------------- unlikely ------------------------ 4d14: 80 61 00 18 lwz r3,24(r1) 4d18: 7f c4 f3 78 mr r4,r30 4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c> * 4d20: 2f 83 00 04 cmpwi cr7,r3,4 4d24: 41 9e 03 94 beq- cr7,50b8 <gfar_clean_rx_ring+0x4a8> 4d28: a0 1f 00 24 lhz r0,36(r31) 4d2c: 81 3f 00 00 lwz r9,0(r31) 4d30: 7f a4 eb 78 mr r4,r29 4d34: 7f e3 fb 78 mr r3,r31 [...] 50b8: 81 3c 01 f8 lwz r9,504(r28) 50bc: 81 5c 01 fc lwz r10,508(r28) 50c0: 31 4a 00 01 addic r10,r10,1 50c4: 7d 29 01 94 addze r9,r9 50c8: 91 3c 01 f8 stw r9,504(r28) 50cc: 91 5c 01 fc stw r10,508(r28) 50d0: 4b ff fc 58 b 4d28 <gfar_clean_rx_ring+0x118> So, the increment does actually get moved ~1k away." Cc: Paul Gortmaker <paul.gortmaker@windriver.com> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 592d297..af0f4d6 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2740,7 +2740,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb, /* Send the packet up the stack */ ret = napi_gro_receive(napi, skb); - if (GRO_DROP == ret) + if (unlikely(GRO_DROP == ret)) atomic64_inc(&priv->extra_stats.kernel_dropped); return 0; -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 5/7] gianfar: gfar_process_frame returns void 2013-02-14 15:00 ` [PATCH net-next 4/7] gianfar: GRO_DROP is unlikely Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 6/7] gianfar: Remove wrong buffer size conditioning to VLAN h/w offload Claudiu Manoil 0 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller No return code is expected from gfar_process_frame(), hence change it to return void. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index af0f4d6..d70f74c 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -133,8 +133,8 @@ static void gfar_netpoll(struct net_device *dev); #endif int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit); static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue); -static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb, - int amount_pull, struct napi_struct *napi); +static void gfar_process_frame(struct net_device *dev, struct sk_buff *skb, + int amount_pull, struct napi_struct *napi); void gfar_halt(struct net_device *dev); static void gfar_halt_nodisable(struct net_device *dev); void gfar_start(struct net_device *dev); @@ -2692,8 +2692,8 @@ static inline void gfar_rx_checksum(struct sk_buff *skb, struct rxfcb *fcb) /* gfar_process_frame() -- handle one incoming packet if skb isn't NULL. */ -static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb, - int amount_pull, struct napi_struct *napi) +static void gfar_process_frame(struct net_device *dev, struct sk_buff *skb, + int amount_pull, struct napi_struct *napi) { struct gfar_private *priv = netdev_priv(dev); struct rxfcb *fcb = NULL; @@ -2742,8 +2742,6 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb, if (unlikely(GRO_DROP == ret)) atomic64_inc(&priv->extra_stats.kernel_dropped); - - return 0; } /* gfar_clean_rx_ring() -- Processes each frame in the rx ring -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 6/7] gianfar: Remove wrong buffer size conditioning to VLAN h/w offload 2013-02-14 15:00 ` [PATCH net-next 5/7] gianfar: gfar_process_frame returns void Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 7/7] gianfar: Fix and cleanup Rx FCB indication Claudiu Manoil 0 siblings, 1 reply; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller The controller's ref manual states clearly that when the hw Rx vlan offload feature is enabled, meaning that the VLEX bit from RCTRL is correctly enabled, then the hw performs automatic VLAN tag extraction and deletion from the ethernet frames. So there's no point in trying to increase the rx buff size when rxvlan is on, as the frame is actually smaller. And the Tx vlan hw accel feature (VLINS) has nothing to do with rx buff size computation. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index d70f74c..b2c6077 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -2385,9 +2385,6 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu) int oldsize = priv->rx_buffer_size; int frame_size = new_mtu + ETH_HLEN; - if (gfar_is_vlan_on(priv)) - frame_size += VLAN_HLEN; - if ((frame_size < 64) || (frame_size > JUMBO_FRAME_SIZE)) { netif_err(priv, drv, dev, "Invalid MTU setting\n"); return -EINVAL; -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 7/7] gianfar: Fix and cleanup Rx FCB indication 2013-02-14 15:00 ` [PATCH net-next 6/7] gianfar: Remove wrong buffer size conditioning to VLAN h/w offload Claudiu Manoil @ 2013-02-14 15:00 ` Claudiu Manoil 0 siblings, 0 replies; 10+ messages in thread From: Claudiu Manoil @ 2013-02-14 15:00 UTC (permalink / raw) To: netdev; +Cc: Paul Gortmaker, David S. Miller This fixes a less obvious error on one hand, and prevents futher similar errors by disambiguating and optimizing RxFCB indication, on the other hand. The error consists in NETIF_F_HW_VLAN_TX flag being used as an indication of Rx FCB insertion. This happened as soon gfar_uses_fcb(), which despite its name indicates Rx FCB insertion, started incorporating is_vlan_on(). is_vlan_on(), on the other hand, is also a misleading construct because we need to differentiate b/w hw VLAN extraction/VLEX (marked by VLAN_RX flag) and hw VLAN insertion/VLINS (VLAN_TX flag), which are different mechanisms using different types of FCBs. The hw spec for the RxFCB feature is as follows: In the case of RxBD rings, FCBs (Frame Control Block) are inserted by the eTSEC whenever RCTRL[PRSDEP] is set to a non-zero value. Only one FCB is inserted per frame (in the buffer pointed to by the RxBD with bit F set). TOE acceleration for receive is enabled for all rx frames in this case. This patch introduces priv->uses_rxfcb field to quickly signal RxFCB insertion in accordance with the specification above. The dependency on FSL_GIANFAR_DEV_HAS_TIMER was also eliminated as another source of confusion. The actual dependency is to priv->hwts_rx_en. Upon changing priv->hwts_rx_en via IOCTL, the gfar device is being restarted and on init_mac() the priv->hwts_rx_en flag determines RxFCB insertion, and rctrl is programmed accordingly. The patch takes care of this case too. Though maybe not as self documenting as the inlining version uses_fcb(), priv->uses_rxfcb has the main purpose to quickly signal, on the hot path, that the incoming frame has a *Rx* FCB block inserted which needs to be pulled out before passing the skb to the stack. This is a performance critical operation, it needs to happen fast, that's why uses_rxfcb is placed in the first cacheline of gfar_private. This is also why a cached rctrl cannot be used instead: 1) because we don't have 32 bits available in the first cacheline of gfar_priv (but only 16); 2) bit operations are expensive on the hot path. Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar.c | 41 ++++++++++++++--------------- drivers/net/ethernet/freescale/gianfar.h | 1 + 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index b2c6077..4b5e8a6 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -344,6 +344,9 @@ static void gfar_init_mac(struct net_device *ndev) /* Configure the coalescing support */ gfar_configure_coalescing(priv, 0xFF, 0xFF); + /* set this when rx hw offload (TOE) functions are being used */ + priv->uses_rxfcb = 0; + if (priv->rx_filer_enable) { rctrl |= RCTRL_FILREN; /* Program the RIR0 reg with the required distribution */ @@ -354,8 +357,10 @@ static void gfar_init_mac(struct net_device *ndev) if (ndev->flags & IFF_PROMISC) rctrl |= RCTRL_PROM; - if (ndev->features & NETIF_F_RXCSUM) + if (ndev->features & NETIF_F_RXCSUM) { rctrl |= RCTRL_CHECKSUMMING; + priv->uses_rxfcb = 1; + } if (priv->extended_hash) { rctrl |= RCTRL_EXTHASH; @@ -377,11 +382,15 @@ static void gfar_init_mac(struct net_device *ndev) } /* Enable HW time stamping if requested from user space */ - if (priv->hwts_rx_en) + if (priv->hwts_rx_en) { rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE; + priv->uses_rxfcb = 1; + } - if (ndev->features & NETIF_F_HW_VLAN_RX) + if (ndev->features & NETIF_F_HW_VLAN_RX) { rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT; + priv->uses_rxfcb = 1; + } /* Init rctrl based on our settings */ gfar_write(®s->rctrl, rctrl); @@ -500,20 +509,6 @@ void unlock_tx_qs(struct gfar_private *priv) spin_unlock(&priv->tx_queue[i]->txlock); } -static bool gfar_is_vlan_on(struct gfar_private *priv) -{ - return (priv->ndev->features & NETIF_F_HW_VLAN_RX) || - (priv->ndev->features & NETIF_F_HW_VLAN_TX); -} - -/* Returns 1 if incoming frames use an FCB */ -static inline int gfar_uses_fcb(struct gfar_private *priv) -{ - return gfar_is_vlan_on(priv) || - (priv->ndev->features & NETIF_F_RXCSUM) || - (priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER); -} - static void free_tx_pointers(struct gfar_private *priv) { int i; @@ -2326,10 +2321,13 @@ void gfar_check_rx_parser_mode(struct gfar_private *priv) tempval = gfar_read(®s->rctrl); /* If parse is no longer required, then disable parser */ - if (tempval & RCTRL_REQ_PARSER) + if (tempval & RCTRL_REQ_PARSER) { tempval |= RCTRL_PRSDEP_INIT; - else + priv->uses_rxfcb = 1; + } else { tempval &= ~RCTRL_PRSDEP_INIT; + priv->uses_rxfcb = 0; + } gfar_write(®s->rctrl, tempval); } @@ -2362,6 +2360,7 @@ void gfar_vlan_mode(struct net_device *dev, netdev_features_t features) tempval = gfar_read(®s->rctrl); tempval |= (RCTRL_VLEX | RCTRL_PRSDEP_INIT); gfar_write(®s->rctrl, tempval); + priv->uses_rxfcb = 1; } else { /* Disable VLAN tag extraction */ tempval = gfar_read(®s->rctrl); @@ -2390,7 +2389,7 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu) return -EINVAL; } - if (gfar_uses_fcb(priv)) + if (priv->uses_rxfcb) frame_size += GMAC_FCB_LEN; frame_size += priv->padding; @@ -2759,7 +2758,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit) bdp = rx_queue->cur_rx; base = rx_queue->rx_bd_base; - amount_pull = (gfar_uses_fcb(priv) ? GMAC_FCB_LEN : 0); + amount_pull = priv->uses_rxfcb ? GMAC_FCB_LEN : 0; while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) { struct sk_buff *newskb; diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 1e2ce8b..63a28d2 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1056,6 +1056,7 @@ struct gfar_private { enum gfar_errata errata; unsigned int rx_buffer_size; + u16 uses_rxfcb; u16 padding; /* HW time stamping enabled flag */ -- 1.7.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path 2013-02-14 15:00 [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private Claudiu Manoil @ 2013-02-14 16:49 ` Paul Gortmaker 2013-02-14 18:32 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: Paul Gortmaker @ 2013-02-14 16:49 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, David S. Miller On 13-02-14 10:00 AM, Claudiu Manoil wrote: > Cc: Paul Gortmaker <paul.gortmaker@windriver.com>, > "David S. Miller" <davem@davemloft.net> > > > Hi Dave, > These patches went already through a review round with Paul. > Changes since the initial version of the patches: > * split the 1st patch into two patches (01 & 02, as suggested by Paul) > * added the suggested trivial cleanup patch: > gianfar: gfar_process_frame returns void > * provided "long logs" with detailed comments and explanations The commit headers do look a lot better, thanks. Passes basic boot test on today's net-next with sbc8548 and NFS root. Paul. -- > > Thank you. > > Regards, > Claudiu > > > Claudiu Manoil (7): > gianfar: Remove unused device_node ref in gfar_private > gianfar: Add device ref (dev) in gfar_private > gianfar: Cleanup and optimize struct gfar_private > gianfar: GRO_DROP is unlikely > gianfar: gfar_process_frame returns void > gianfar: Remove wrong buffer size conditioning to VLAN h/w offload > gianfar: Fix and cleanup Rx FCB indication > > drivers/net/ethernet/freescale/gianfar.c | 85 ++++++++++++--------------- > drivers/net/ethernet/freescale/gianfar.h | 96 +++++++++++++++-------------- > 2 files changed, 88 insertions(+), 93 deletions(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path 2013-02-14 15:00 [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private Claudiu Manoil 2013-02-14 16:49 ` [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Paul Gortmaker @ 2013-02-14 18:32 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2013-02-14 18:32 UTC (permalink / raw) To: claudiu.manoil; +Cc: netdev, paul.gortmaker From: Claudiu Manoil <claudiu.manoil@freescale.com> Date: Thu, 14 Feb 2013 17:00:00 +0200 > These patches went already through a review round with Paul. > Changes since the initial version of the patches: > * split the 1st patch into two patches (01 & 02, as suggested by Paul) > * added the suggested trivial cleanup patch: > gianfar: gfar_process_frame returns void > * provided "long logs" with detailed comments and explanations Series applied, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-02-14 18:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-14 15:00 [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 2/7] gianfar: Add device ref (dev) " Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 3/7] gianfar: Cleanup and optimize struct gfar_private Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 4/7] gianfar: GRO_DROP is unlikely Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 5/7] gianfar: gfar_process_frame returns void Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 6/7] gianfar: Remove wrong buffer size conditioning to VLAN h/w offload Claudiu Manoil 2013-02-14 15:00 ` [PATCH net-next 7/7] gianfar: Fix and cleanup Rx FCB indication Claudiu Manoil 2013-02-14 16:49 ` [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path Paul Gortmaker 2013-02-14 18:32 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.