| Message ID | 1401951572-6538-2-git-send-email-b38611@freescale.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Fugang Duan > Make the code more readable and easy to support other features like > SG, TSO, moving the common transmit function to one api. > > And the patch also factorize the getting BD index to it own function. > > Signed-off-by: Fugang Duan <B38611@freescale.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 87 ++++++++++++++++++----------- > 1 files changed, 54 insertions(+), 33 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 802be17..32c2276 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct fec_enet_priva > return (new_bd < base) ? (new_bd + ring_size) : new_bd; > } > > +static inline > +int fec_enet_get_bd_index(struct bufdesc *bdp, struct fec_enet_private *fep) > +{ > + struct bufdesc *base; > + int index; > + > + if (bdp >= fep->tx_bd_base) > + base = fep->tx_bd_base; > + else > + base = fep->rx_bd_base; You really don't want the above conditional. It is known from the call site - but the compiler can't determine that and remove the test. You may not want to rely on the tx and rx descriptors being allocated as a single entity - particularly now that they (probably) consume more than a single page. > + > + if (fep->bufdesc_ex) > + index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex *)base; > + else > + index = bdp - base; Save the sizeof the descriptor structure as (say) fep->bufdesc_size (Possibly replacing bufdesc_ex) and then just calculate: index = ((const char *)bdp - (const char *)base)/fep->bufdesc_size; David -- 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
From: David Laight <David.Laight@ACULAB.COM> Data: Thursday, June 05, 2014 5:07 PM > To: Duan Fugang-B38611; davem@davemloft.net > Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; Estevam Fabio-R49496; > ezequiel.garcia@free-electrons.com; bhutchings@solarflare.com; > stephen@networkplumber.org; Li Frank-B20596; eric.dumazet@gmail.com > Subject: RE: [PATCH v3 1/6] net: fec: Factorize the .xmit transmit function > > From: Fugang Duan > > Make the code more readable and easy to support other features like > > SG, TSO, moving the common transmit function to one api. > > > > And the patch also factorize the getting BD index to it own function. > > > > Signed-off-by: Fugang Duan <B38611@freescale.com> > > --- > > drivers/net/ethernet/freescale/fec_main.c | 87 ++++++++++++++++++----- > ------ > > 1 files changed, 54 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > b/drivers/net/ethernet/freescale/fec_main.c > > index 802be17..32c2276 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct bufdesc > *bdp, struct fec_enet_priva > > return (new_bd < base) ? (new_bd + ring_size) : new_bd; } > > > > +static inline > > +int fec_enet_get_bd_index(struct bufdesc *bdp, struct > > +fec_enet_private *fep) { > > + struct bufdesc *base; > > + int index; > > + > > + if (bdp >= fep->tx_bd_base) > > + base = fep->tx_bd_base; > > + else > > + base = fep->rx_bd_base; > > You really don't want the above conditional. > It is known from the call site - but the compiler can't determine that and > remove the test. Can you give me more training for this ? Sorry, I don't understand the means. > You may not want to rely on the tx and rx descriptors being allocated as a > single entity - particularly now that they (probably) consume more than a > single page. > BDs consumes more than single page, but it use " dma_alloc_coherent()" to allocate continuous memory, I don't know why it is cannot do it like this. > > + > > + if (fep->bufdesc_ex) > > + index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex *)base; > > + else > > + index = bdp - base; > > Save the sizeof the descriptor structure as (say) fep->bufdesc_size > (Possibly replacing bufdesc_ex) and then just calculate: > index = ((const char *)bdp - (const char *)base)/fep->bufdesc_size; > > David > You means: if (fep->bufdesc_ex) bufdesc_size = sizeof(struct bufdesc_ex); else bufdesc_size = sizeof(struct bufdesc); index = ((const char *)bdp - (const char *)base)/bufdesc_size; Thanks, Andy -- 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
From: fugang.duan@freescale. > > From: Fugang Duan > > > Make the code more readable and easy to support other features like > > > SG, TSO, moving the common transmit function to one api. > > > > > > And the patch also factorize the getting BD index to it own function. > > > > > > Signed-off-by: Fugang Duan <B38611@freescale.com> > > > --- > > > drivers/net/ethernet/freescale/fec_main.c | 87 ++++++++++++++++++----- > > ------ > > > 1 files changed, 54 insertions(+), 33 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > > b/drivers/net/ethernet/freescale/fec_main.c > > > index 802be17..32c2276 100644 > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > @@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct bufdesc > > *bdp, struct fec_enet_priva > > > return (new_bd < base) ? (new_bd + ring_size) : new_bd; } > > > > > > +static inline > > > +int fec_enet_get_bd_index(struct bufdesc *bdp, struct > > > +fec_enet_private *fep) { > > > + struct bufdesc *base; > > > + int index; > > > + > > > + if (bdp >= fep->tx_bd_base) > > > + base = fep->tx_bd_base; > > > + else > > > + base = fep->rx_bd_base; > > > > You really don't want the above conditional. > > It is known from the call site - but the compiler can't determine that and > > remove the test. > Can you give me more training for this ? Sorry, I don't understand the means. Single 'if' statements can have measurable performance impact on ethernet code paths - so it isn't a good idea to add ones that aren't strictly required. If the function had an extra argument 'tx_ring' (don't add one) which would be constant at all the call sites and the code read: base = tx_ring ? fep->tx_bd_base : fep->rx_bd_base; when the compiler inlines the function calls it would be able to optimise away the conditional. > > You may not want to rely on the tx and rx descriptors being allocated as a > > single entity - particularly now that they (probably) consume more than a > > single page. > > > BDs consumes more than single page, but it use " dma_alloc_coherent()" to allocate continuous memory, > I don't know why it is cannot do it like this. > > > > + > > > + if (fep->bufdesc_ex) > > > + index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex *)base; > > > + else > > > + index = bdp - base; > > > > Save the sizeof the descriptor structure as (say) fep->bufdesc_size > > (Possibly replacing bufdesc_ex) and then just calculate: > > index = ((const char *)bdp - (const char *)base)/fep->bufdesc_size; > > > > David > > > You means: > if (fep->bufdesc_ex) > bufdesc_size = sizeof(struct bufdesc_ex); > else > bufdesc_size = sizeof(struct bufdesc); > index = ((const char *)bdp - (const char *)base)/bufdesc_size; No, that is still a conditional in the normal path. Assign fep->bufdesc_size during initialisation. Then end up with something like: static int fec_enet_get_bd_index(struct bufdesc *base, struct bufdesc *bdp, struct fec_enet_private *fep) { return ((const char *)bdp - (const char *)base)/fep->bufdesc_size; } David -- 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
Hi, David, From: David Laight David.Laight@ACULAB.COM> Data: Thursday, June 05, 2014 5:58 PM > To: Duan Fugang-B38611; davem@davemloft.net > Cc: netdev@vger.kernel.org; shawn.guo@linaro.org; Estevam Fabio-R49496; > ezequiel.garcia@free-electrons.com; bhutchings@solarflare.com; > stephen@networkplumber.org; Li Frank-B20596; eric.dumazet@gmail.com > Subject: RE: [PATCH v3 1/6] net: fec: Factorize the .xmit transmit > function > > From: fugang.duan@freescale. > > > From: Fugang Duan > > > > Make the code more readable and easy to support other features > > > > like SG, TSO, moving the common transmit function to one api. > > > > > > > > And the patch also factorize the getting BD index to it own > function. > > > > > > > > Signed-off-by: Fugang Duan <B38611@freescale.com> > > > > --- > > > > drivers/net/ethernet/freescale/fec_main.c | 87 > ++++++++++++++++++----- > > > ------ > > > > 1 files changed, 54 insertions(+), 33 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > > > b/drivers/net/ethernet/freescale/fec_main.c > > > > index 802be17..32c2276 100644 > > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > > @@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct > > > > bufdesc > > > *bdp, struct fec_enet_priva > > > > return (new_bd < base) ? (new_bd + ring_size) : > new_bd; } > > > > > > > > +static inline > > > > +int fec_enet_get_bd_index(struct bufdesc *bdp, struct > > > > +fec_enet_private *fep) { > > > > + struct bufdesc *base; > > > > + int index; > > > > + > > > > + if (bdp >= fep->tx_bd_base) > > > > + base = fep->tx_bd_base; > > > > + else > > > > + base = fep->rx_bd_base; > > > > > > You really don't want the above conditional. > > > It is known from the call site - but the compiler can't determine > > > that and remove the test. > > > Can you give me more training for this ? Sorry, I don't understand > the means. > > Single 'if' statements can have measurable performance impact on > ethernet code paths - so it isn't a good idea to add ones that aren't > strictly required. > > If the function had an extra argument 'tx_ring' (don't add one) which > would be constant at all the call sites and the code read: > base = tx_ring ? fep->tx_bd_base : fep->rx_bd_base; when the > compiler inlines the function calls it would be able to optimise away > the conditional. > > > > You may not want to rely on the tx and rx descriptors being > > > allocated as a single entity - particularly now that they (probably) > > > consume more than a single page. > > > > > BDs consumes more than single page, but it use " > dma_alloc_coherent()" > > to allocate continuous memory, I don't know why it is cannot do it > like this. > > > > > > + > > > > + if (fep->bufdesc_ex) > > > > + index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex > *)base; > > > > + else > > > > + index = bdp - base; > > > > > > Save the sizeof the descriptor structure as (say) fep->bufdesc_size > > > (Possibly replacing bufdesc_ex) and then just calculate: > > > index = ((const char *)bdp - (const char *)base)/fep- > >bufdesc_size; > > > > > > David > > > > > You means: > > if (fep->bufdesc_ex) > > bufdesc_size = sizeof(struct bufdesc_ex); else > > bufdesc_size = sizeof(struct bufdesc); index = ((const char *)bdp > - > > (const char *)base)/bufdesc_size; > > No, that is still a conditional in the normal path. > Assign fep->bufdesc_size during initialisation. > > Then end up with something like: > static > int fec_enet_get_bd_index(struct bufdesc *base, struct bufdesc *bdp, > struct fec_enet_private *fep) > { > return ((const char *)bdp - (const char *)base)/fep- > >bufdesc_size; } > > David > You think the conditional has measurable performance impact on ethernet code paths, I agree your thinking. But in the driver, there have many site/apis use the method. So, I don't want to change it for the patch set. It have no meaningful. I will submit another patch to solve all APIs with conditional for the driver. Thanks, Andy -- 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
From: "fugang.duan@freescale.com" <fugang.duan@freescale.com> Date: Thu, 5 Jun 2014 10:21:37 +0000 > You think the conditional has measurable performance impact on > ethernet code paths, I agree your thinking. But in the driver, > there have many site/apis use the method. > > So, I don't want to change it for the patch set. It have no meaningful. > I will submit another patch to solve all APIs with conditional for the driver. You're adding this test, it did not exist beforehand. For the second time I'm asking you to follow David Laight's feedback seriously, every time he asks you to make a change your initial reaction is to not do it, or to do it "later". If it's for something this patch set uniquely is doing, "later" is not an acceptable response. I'm not applying this series until you intergrate his legitimate feedback. Thank you. -- 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
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 802be17..32c2276 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -287,6 +287,25 @@ struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct fec_enet_priva return (new_bd < base) ? (new_bd + ring_size) : new_bd; } +static inline +int fec_enet_get_bd_index(struct bufdesc *bdp, struct fec_enet_private *fep) +{ + struct bufdesc *base; + int index; + + if (bdp >= fep->tx_bd_base) + base = fep->tx_bd_base; + else + base = fep->rx_bd_base; + + if (fep->bufdesc_ex) + index = (struct bufdesc_ex *)bdp - (struct bufdesc_ex *)base; + else + index = bdp - base; + + return index; +} + static void *swap_buffer(void *bufaddr, int len) { int i; @@ -313,8 +332,7 @@ fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev) return 0; } -static netdev_tx_t -fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) +static int txq_submit_skb(struct sk_buff *skb, struct net_device *ndev) { struct fec_enet_private *fep = netdev_priv(ndev); const struct platform_device_id *id_entry = @@ -329,14 +347,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) status = bdp->cbd_sc; - if (status & BD_ENET_TX_READY) { - /* Ooops. All transmit buffers are full. Bail out. - * This should not happen, since ndev->tbusy should be set. - */ - netdev_err(ndev, "tx queue full!\n"); - return NETDEV_TX_BUSY; - } - /* Protocol checksum off-load for TCP and UDP. */ if (fec_enet_clear_csum(skb, ndev)) { dev_kfree_skb_any(skb); @@ -350,16 +360,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) bufaddr = skb->data; bdp->cbd_datlen = skb->len; - /* - * On some FEC implementations data must be aligned on - * 4-byte boundaries. Use bounce buffers to copy data - * and get it aligned. Ugh. - */ - if (fep->bufdesc_ex) - index = (struct bufdesc_ex *)bdp - - (struct bufdesc_ex *)fep->tx_bd_base; - else - index = bdp - fep->tx_bd_base; + index = fec_enet_get_bd_index(bdp, fep); if (((unsigned long) bufaddr) & FEC_ALIGNMENT) { memcpy(fep->tx_bounce[index], skb->data, skb->len); @@ -433,15 +434,43 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) fep->cur_tx = bdp; - if (fep->cur_tx == fep->dirty_tx) - netif_stop_queue(ndev); - /* Trigger transmission start */ writel(0, fep->hwp + FEC_X_DES_ACTIVE); return NETDEV_TX_OK; } +static netdev_tx_t +fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) +{ + struct fec_enet_private *fep = netdev_priv(ndev); + struct bufdesc *bdp; + unsigned short status; + int ret; + + /* Fill in a Tx ring entry */ + bdp = fep->cur_tx; + + status = bdp->cbd_sc; + + if (status & BD_ENET_TX_READY) { + /* Ooops. All transmit buffers are full. Bail out. + * This should not happen, since ndev->tbusy should be set. + */ + netdev_err(ndev, "tx queue full!\n"); + return NETDEV_TX_BUSY; + } + + ret = txq_submit_skb(skb, ndev); + if (ret == -EBUSY) + return NETDEV_TX_BUSY; + + if (fep->cur_tx == fep->dirty_tx) + netif_stop_queue(ndev); + + return NETDEV_TX_OK; +} + /* Init RX & TX buffer descriptors */ static void fec_enet_bd_init(struct net_device *dev) @@ -770,11 +799,7 @@ fec_enet_tx(struct net_device *ndev) if (bdp == fep->cur_tx) break; - if (fep->bufdesc_ex) - index = (struct bufdesc_ex *)bdp - - (struct bufdesc_ex *)fep->tx_bd_base; - else - index = bdp - fep->tx_bd_base; + index = fec_enet_get_bd_index(bdp, fep); skb = fep->tx_skbuff[index]; dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr, skb->len, @@ -921,11 +946,7 @@ fec_enet_rx(struct net_device *ndev, int budget) pkt_len = bdp->cbd_datlen; ndev->stats.rx_bytes += pkt_len; - if (fep->bufdesc_ex) - index = (struct bufdesc_ex *)bdp - - (struct bufdesc_ex *)fep->rx_bd_base; - else - index = bdp - fep->rx_bd_base; + index = fec_enet_get_bd_index(bdp, fep); data = fep->rx_skbuff[index]->data; dma_sync_single_for_cpu(&fep->pdev->dev, bdp->cbd_bufaddr, FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
Make the code more readable and easy to support other features like SG, TSO, moving the common transmit function to one api. And the patch also factorize the getting BD index to it own function. Signed-off-by: Fugang Duan <B38611@freescale.com> --- drivers/net/ethernet/freescale/fec_main.c | 87 ++++++++++++++++++----------- 1 files changed, 54 insertions(+), 33 deletions(-)