RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] On Wed, 2015-06-10 at 07:38 +, Madalin-Cristian Bucur wrote: Hi Eric, Can you please tell us if this change would be for the better? I was about to say yes to this request but checked and no other Ethernet driver seems to use the queue trans_start. I was able to find your patch net: tx scalability works : trans_start [ http://patchwork.ozlabs.org/patch/27104/ ] but did not find more about this topic. Drivers no longer have to worry about updating trans_start themselves. Core networking stack handles this. Check txq_trans_update() done from netdev_start_xmit() I'll remove the updates, thank you. Madalin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
-Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] On Wed, 2015-04-29 at 17:56 +0300, Madalin Bucur wrote: This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms. ... + /* We're going to store the skb backpointer at the beginning +* of the data buffer, so we need a privately owned skb +*/ + + /* Code borrowed from skb_unshare(). */ + if (skb_cloned(skb)) { + struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC); + + /* Finally, create a contig FD from this skb */ + skb_to_contig_fd(priv, skb, fd, countptr, offset); + + kfree_skb(skb); + skb = nskb; + /* skb_copy() has now linearized the skbuff. */ + } + You know that TCP packets are clones, right ? This code is killing performance. TCP allows the headers part being modified by a driver if needed. You should use skb_header_cloned() instead. Thank you, I'll address this. I plan to do something like this: + if (!nonlinear) { + /* We're going to store the skb backpointer at the beginning +* of the data buffer, so we need a privately owned skb +*/ + + /* make sure skb is not shared, skb_cow_head() assumes it's not */ + skb = skb_share_check(skb, GFP_ATOMIC); + if (!skb) + goto enomem; + + /* verify the skb head is not cloned */ + if (skb_cow_head(skb, priv-tx_headroom)) + goto enomem; + + nonlinear = skb_is_nonlinear(skb); + } but I'm not sure the skb_share_check() is required on tx. I'm also a bit puzzled by the aliasing between shared and cloned terms (i.e. skb_unshare() could be named something like skb_unclone(); the skb_share_check() not only checks but also unshares an skb so the already taken skb_unshare() name would probably fit too). Madalin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
On Mon, 2015-06-15 at 13:40 +, Madalin-Cristian Bucur wrote: -Original Message- From: Eric Dumazet [mailto:eric.duma...@gmail.com] On Wed, 2015-04-29 at 17:56 +0300, Madalin Bucur wrote: This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms. ... + /* We're going to store the skb backpointer at the beginning + * of the data buffer, so we need a privately owned skb + */ + + /* Code borrowed from skb_unshare(). */ + if (skb_cloned(skb)) { + struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC); + + /* Finally, create a contig FD from this skb */ + skb_to_contig_fd(priv, skb, fd, countptr, offset); + + kfree_skb(skb); + skb = nskb; + /* skb_copy() has now linearized the skbuff. */ + } + You know that TCP packets are clones, right ? This code is killing performance. TCP allows the headers part being modified by a driver if needed. You should use skb_header_cloned() instead. Thank you, I'll address this. I plan to do something like this: + if (!nonlinear) { + /* We're going to store the skb backpointer at the beginning +* of the data buffer, so we need a privately owned skb +*/ + + /* make sure skb is not shared, skb_cow_head() assumes it's not */ + skb = skb_share_check(skb, GFP_ATOMIC); + if (!skb) + goto enomem; + + /* verify the skb head is not cloned */ + if (skb_cow_head(skb, priv-tx_headroom)) + goto enomem; + + nonlinear = skb_is_nonlinear(skb); + } but I'm not sure the skb_share_check() is required on tx. I'm also a bit puzzled by the aliasing between shared and cloned terms (i.e. skb_unshare() could be named something like skb_unclone(); the skb_share_check() not only checks but also unshares an skb so the already taken skb_unshare() name would probably fit too). A driver can ask tx skbs to be not shared, even if pktgen is used with clone=X on it. dev-priv_flags = ~IFF_TX_SKB_SHARING; This way, you can avoid skb_share_check() completely. Only pktgen will care. Most skb_share_check() calls in drivers/net are related to input paths on virtual drivers. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
Hi Eric, Can you please tell us if this change would be for the better? I was about to say yes to this request but checked and no other Ethernet driver seems to use the queue trans_start. I was able to find your patch net: tx scalability works : trans_start [ http://patchwork.ozlabs.org/patch/27104/ ] but did not find more about this topic. Thank you, Madalin -Original Message- From: Xie Jianhua-B29408 Sent: Wednesday, June 10, 2015 9:00 AM To: Bucur Madalin-Cristian-B32716; net...@vger.kernel.org; linuxppc- d...@lists.ozlabs.org Cc: linux-ker...@vger.kernel.org Subject: RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet -Original Message- From: Linuxppc-dev [mailto:linuxppc-dev- bounces+jianhua.xie=freescale@lists.ozlabs.org] On Behalf Of Madalin Bucur Sent: Wednesday, April 29, 2015 10:57 PM To: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org Cc: linux-ker...@vger.kernel.org; Bucur Madalin-Cristian-B32716 Subject: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms. Snip.. + + if (unlikely(dpa_xmit(priv, percpu_stats, queue_mapping, fd) 0)) + goto xmit_failed; + + net_dev-trans_start = jiffies; It is probably better to use netdev_queue-trans_start to instead of net_dev-trans_start on SMP. Best Regards, Jianhua + return NETDEV_TX_OK; + +xmit_failed: + if (fd.cmd FM_FD_CMD_FCO) { + (*countptr)--; + dpa_fd_release(net_dev, fd); + percpu_stats-tx_errors++; + return NETDEV_TX_OK; + } + _dpa_cleanup_tx_fd(priv, fd); + percpu_stats-tx_errors++; + dev_kfree_skb(skb); + return NETDEV_TX_OK; +} -- 1.7.11.7 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
On Wed, 2015-06-10 at 07:38 +, Madalin-Cristian Bucur wrote: Hi Eric, Can you please tell us if this change would be for the better? I was about to say yes to this request but checked and no other Ethernet driver seems to use the queue trans_start. I was able to find your patch net: tx scalability works : trans_start [ http://patchwork.ozlabs.org/patch/27104/ ] but did not find more about this topic. Drivers no longer have to worry about updating trans_start themselves. Core networking stack handles this. Check txq_trans_update() done from netdev_start_xmit() ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
-Original Message- From: Linuxppc-dev [mailto:linuxppc-dev- bounces+jianhua.xie=freescale@lists.ozlabs.org] On Behalf Of Madalin Bucur Sent: Wednesday, April 29, 2015 10:57 PM To: net...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org Cc: linux-ker...@vger.kernel.org; Bucur Madalin-Cristian-B32716 Subject: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms. Snip.. + + if (unlikely(dpa_xmit(priv, percpu_stats, queue_mapping, fd) 0)) + goto xmit_failed; + + net_dev-trans_start = jiffies; It is probably better to use netdev_queue-trans_start to instead of net_dev-trans_start on SMP. Best Regards, Jianhua + return NETDEV_TX_OK; + +xmit_failed: + if (fd.cmd FM_FD_CMD_FCO) { + (*countptr)--; + dpa_fd_release(net_dev, fd); + percpu_stats-tx_errors++; + return NETDEV_TX_OK; + } + _dpa_cleanup_tx_fd(priv, fd); + percpu_stats-tx_errors++; + dev_kfree_skb(skb); + return NETDEV_TX_OK; +} -- 1.7.11.7 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC,v3 02/10] dpaa_eth: add support for DPAA Ethernet
On Wed, 2015-04-29 at 17:56 +0300, Madalin Bucur wrote: This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms. ... + /* We're going to store the skb backpointer at the beginning + * of the data buffer, so we need a privately owned skb + */ + + /* Code borrowed from skb_unshare(). */ + if (skb_cloned(skb)) { + struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC); + + /* Finally, create a contig FD from this skb */ + skb_to_contig_fd(priv, skb, fd, countptr, offset); + + kfree_skb(skb); + skb = nskb; + /* skb_copy() has now linearized the skbuff. */ + } + You know that TCP packets are clones, right ? This code is killing performance. TCP allows the headers part being modified by a driver if needed. You should use skb_header_cloned() instead. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev