[PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Joakim Zhang]

This patch set intends to fix TX bandwidth fluctuations, any feedback would be appreciated.

---
ChangeLogs:
	V1: remove RFC tag, RFC discussions please turn to below:
	    https://lore.kernel.org/lkml/YK0Ce5YxR2WYbrAo@lunn.ch/T/

Fugang Duan (1):
  net: fec: add ndo_select_queue to fix TX bandwidth fluctuations

Joakim Zhang (1):
  net: fec: add FEC_QUIRK_HAS_MULTI_QUEUES represents i.MX6SX ENET IP

 drivers/net/ethernet/freescale/fec.h      |  5 +++
 drivers/net/ethernet/freescale/fec_main.c | 43 ++++++++++++++++++++---
 2 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH V1 net-next 1/2] net: fec: add FEC_QUIRK_HAS_MULTI_QUEUES represents i.MX6SX ENET IP

[Joakim Zhang]

Frieder Schrempf reported a TX throuthput issue [1], it happens quite often
that the measured bandwidth in TX direction drops from its expected/nominal
value to something like ~50% (for 100M) or ~67% (for 1G) connections.

[1] https://lore.kernel.org/linux-arm-kernel/421cc86c-b66f-b372-32f7-21e59f9a98bc@kontron.de/

The issue becomes clear after digging into it, Net core would select
queues when transmitting packets. Since FEC have not impletemented
ndo_select_queue callback yet, so it will call netdev_pick_tx to select
queues randomly.

For i.MX6SX ENET IP with AVB support, driver default enables this
feature. According to the setting of QOS/RCMRn/DMAnCFG registers, AVB
configured to Credit-based scheme, 50% bandwidth of each queue 1&2.

With below tests let me think more:
1) With FEC_QUIRK_HAS_AVB quirk, can reproduce TX bandwidth fluctuations issue.
2) Without FEC_QUIRK_HAS_AVB quirk, can't reproduce TX bandwidth fluctuations issue.

The related difference with or w/o FEC_QUIRK_HAS_AVB quirk is that, whether we
program FTYPE field of TxBD or not. As I describe above, AVB feature is
enabled by default. With FEC_QUIRK_HAS_AVB quirk, frames in queue 0
marked as non-AVB, and frames in queue 1&2 marked as AVB Class A&B. It's
unreasonable if frames in queue 1&2 are not required to be time-sensitive.
So when Net core select tx queues ramdomly, Credit-based scheme would work
and lead to TX bandwidth fluctuated. On the other hand, w/o
FEC_QUIRK_HAS_AVB quirk, frames in queue 1&2 are all marked as non-AVB, so
Credit-based scheme would not work.

Till now, how can we fix this TX throughput issue? Yes, please remove
FEC_QUIRK_HAS_AVB quirk if you suffer it from time-nonsensitive networking.
However, this quirk is used to indicate i.MX6SX, other setting depends
on it. So this patch adds a new quirk FEC_QUIRK_HAS_MULTI_QUEUES to
represent i.MX6SX, it is safe for us remove FEC_QUIRK_HAS_AVB quirk
now.

FEC_QUIRK_HAS_AVB quirk is set by default in the driver, and users may
not know much about driver details, they would waste effort to find the
root cause, that is not we want. The following patch is a implementation
to fix it and users don't need to modify the driver.

Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/ethernet/freescale/fec.h      |  5 +++++
 drivers/net/ethernet/freescale/fec_main.c | 11 ++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 0602d5d5d2ee..2e002e4b4b4a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -467,6 +467,11 @@ struct bufdesc_ex {
  */
 #define FEC_QUIRK_NO_HARD_RESET		(1 << 18)
 
+/* i.MX6SX ENET IP supports multiple queues (3 queues), use this quirk to
+ * represents this ENET IP.
+ */
+#define FEC_QUIRK_HAS_MULTI_QUEUES	(1 << 19)
+
 struct bufdesc_prop {
 	int qid;
 	/* Address of Rx and Tx buffers */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f2065f9d02e6..053a0e547e4f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -122,7 +122,7 @@ static const struct fec_devinfo fec_imx6x_info = {
 		  FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
 		  FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE |
 		  FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE |
-		  FEC_QUIRK_CLEAR_SETUP_MII,
+		  FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES,
 };
 
 static const struct fec_devinfo fec_imx6ul_info = {
@@ -421,6 +421,7 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 				estatus |= FEC_TX_BD_FTYPE(txq->bd.qid);
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
 				estatus |= BD_ENET_TX_PINS | BD_ENET_TX_IINS;
+
 			ebdp->cbd_bdu = 0;
 			ebdp->cbd_esc = cpu_to_fec32(estatus);
 		}
@@ -954,7 +955,7 @@ fec_restart(struct net_device *ndev)
 	 * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
 	 * instead of reset MAC itself.
 	 */
-	if (fep->quirks & FEC_QUIRK_HAS_AVB ||
+	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES ||
 	    ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
 		writel(0, fep->hwp + FEC_ECNTRL);
 	} else {
@@ -1165,7 +1166,7 @@ fec_stop(struct net_device *ndev)
 	 * instead of reset MAC itself.
 	 */
 	if (!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) {
-		if (fep->quirks & FEC_QUIRK_HAS_AVB) {
+		if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
 			writel(0, fep->hwp + FEC_ECNTRL);
 		} else {
 			writel(1, fep->hwp + FEC_ECNTRL);
@@ -2566,7 +2567,7 @@ static void fec_enet_itr_coal_set(struct net_device *ndev)
 
 	writel(tx_itr, fep->hwp + FEC_TXIC0);
 	writel(rx_itr, fep->hwp + FEC_RXIC0);
-	if (fep->quirks & FEC_QUIRK_HAS_AVB) {
+	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
 		writel(tx_itr, fep->hwp + FEC_TXIC1);
 		writel(rx_itr, fep->hwp + FEC_RXIC1);
 		writel(tx_itr, fep->hwp + FEC_TXIC2);
@@ -3361,7 +3362,7 @@ static int fec_enet_init(struct net_device *ndev)
 		fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
 	}
 
-	if (fep->quirks & FEC_QUIRK_HAS_AVB) {
+	if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES) {
 		fep->tx_align = 0;
 		fep->rx_align = 0x3f;
 	}
-- 
2.17.1

[PATCH V1 net-next 2/2] net: fec: add ndo_select_queue to fix TX bandwidth fluctuations

[Joakim Zhang]

From: Fugang Duan <fugang.duan@nxp.com>

As we know that AVB is enabled by default, and the ENET IP design is
queue 0 for best effort, queue 1&2 for AVB Class A&B. Bandwidth of each
queue 1&2 set in driver is 50%, TX bandwidth fluctuated when selecting
tx queues randomly with FEC_QUIRK_HAS_AVB quirk available.

This patch adds ndo_select_queue callback to select queues for
transmitting to fix this issue. It will always return queue 0 if this is
not a vlan packet, and return queue 1 or 2 based on priority of vlan
packet.

You may complain that in fact we only use single queue for trasmitting
if we are not targeted to VLAN. Yes, but seems we have no choice, since
AVB is enabled when the driver probed, we can't switch this feature
dynamicly. After compare multiple queues to single queue, TX throughput
almost no improvement.

One way we can implemet is to configure the driver to multiple queues
with Round-robin scheme by default. Then add ndo_setup_tc callback to
enable/disable AVB feature for users. Unfortunately, ENET AVB IP seems
not follow the standard 802.1Qav spec. We only can program
DMAnCFG[IDLE_SLOPE] field to calculate bandwidth fraction. And idle
slope is restricted to certain valus (a total of 19). It's far away from
CBS QDisc implemented in Linux TC framework. If you strongly suggest to do
this, I think we only can support limited numbers of bandwidth and reject
others, but it's really urgly and wried.

With this patch, VLAN tagged packets route to queue 0/1/2 based on vlan
priority; VLAN untagged packets route to queue 0.

Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 32 +++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 053a0e547e4f..6d7e2db17995 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -76,6 +76,8 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
 
 #define DRIVER_NAME	"fec"
 
+static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2};
+
 /* Pause frame feild and FIFO threshold */
 #define FEC_ENET_FCE	(1 << 5)
 #define FEC_ENET_RSEM_V	0x84
@@ -3236,10 +3238,40 @@ static int fec_set_features(struct net_device *netdev,
 	return 0;
 }
 
+u16 fec_enet_get_raw_vlan_tci(struct sk_buff *skb)
+{
+	struct vlan_ethhdr *vhdr;
+	unsigned short vlan_TCI = 0;
+
+	if (skb->protocol == ntohs(ETH_P_ALL)) {
+		vhdr = (struct vlan_ethhdr *)(skb->data);
+		vlan_TCI = ntohs(vhdr->h_vlan_TCI);
+	}
+
+	return vlan_TCI;
+}
+
+u16 fec_enet_select_queue(struct net_device *ndev, struct sk_buff *skb,
+			  struct net_device *sb_dev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	u16 vlan_tag;
+
+	if (!(fep->quirks & FEC_QUIRK_HAS_AVB))
+		return netdev_pick_tx(ndev, skb, NULL);
+
+	vlan_tag = fec_enet_get_raw_vlan_tci(skb);
+	if (!vlan_tag)
+		return vlan_tag;
+
+	return fec_enet_vlan_pri_to_queue[vlan_tag >> 13];
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
 	.ndo_start_xmit		= fec_enet_start_xmit,
+	.ndo_select_queue       = fec_enet_select_queue,
 	.ndo_set_rx_mode	= set_multicast_list,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_tx_timeout		= fec_timeout,
-- 
2.17.1

Re: [PATCH V1 net-next 2/2] net: fec: add ndo_select_queue to fix TX bandwidth fluctuations

[Jakub Kicinski]

On Thu, 27 May 2021 20:07:22 +0800 Joakim Zhang wrote:
> +u16 fec_enet_get_raw_vlan_tci(struct sk_buff *skb)

Let's perhaps wait for the testing with a repost but both functions
added in this patch need to be static.

RE: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Joakim Zhang]

Hi Frieder,

As we talked before, could you please help test the patches when you are free? Thanks.

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Joakim Zhang <qiangqing.zhang@nxp.com>
> Sent: 2021年5月27日 20:07
> To: davem@davemloft.net; kuba@kernel.org; frieder.schrempf@kontron.de;
> andrew@lunn.ch
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations
> 
> This patch set intends to fix TX bandwidth fluctuations, any feedback would be
> appreciated.
> 
> ---
> ChangeLogs:
> 	V1: remove RFC tag, RFC discussions please turn to below:
> 	    https://lore.kernel.org/lkml/YK0Ce5YxR2WYbrAo@lunn.ch/T/
> 
> Fugang Duan (1):
>   net: fec: add ndo_select_queue to fix TX bandwidth fluctuations
> 
> Joakim Zhang (1):
>   net: fec: add FEC_QUIRK_HAS_MULTI_QUEUES represents i.MX6SX ENET IP
> 
>  drivers/net/ethernet/freescale/fec.h      |  5 +++
>  drivers/net/ethernet/freescale/fec_main.c | 43 ++++++++++++++++++++---
>  2 files changed, 43 insertions(+), 5 deletions(-)
> 
> --
> 2.17.1

Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Andrew Lunn]

On Thu, May 27, 2021 at 12:10:47PM +0000, Joakim Zhang wrote:
> 
> Hi Frieder,
> 
> As we talked before, could you please help test the patches when you are free? Thanks.

Hi Frieder

If you can, could you also test it with traffic with a mixture of VLAN
priorities. You might want to force the link to 10Full, so you can
overload it. Then see what traffic actually makes it through.

	 Andrew

RE: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Joakim Zhang]

Hi Frieder,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年5月27日 22:06
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: davem@davemloft.net; kuba@kernel.org; frieder.schrempf@kontron.de;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations
> 
> On Thu, May 27, 2021 at 12:10:47PM +0000, Joakim Zhang wrote:
> >
> > Hi Frieder,
> >
> > As we talked before, could you please help test the patches when you are
> free? Thanks.
> 
> Hi Frieder
> 
> If you can, could you also test it with traffic with a mixture of VLAN priorities.
> You might want to force the link to 10Full, so you can overload it. Then see
> what traffic actually makes it through.

Did your mailbox get bombed, let you miss this mail, hope you can see this reply.

Could you please give some feedback if it is possible? Thanks :-)

Best Regards,
Joakim Zhang
> 	 Andrew

Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Frieder Schrempf]

Hi Joakim, hi Andrew,

On 08.06.21 05:23, Joakim Zhang wrote:
> 
> Hi Frieder,
> 
>> -----Original Message-----
>> From: Andrew Lunn <andrew@lunn.ch>
>> Sent: 2021年5月27日 22:06
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>
>> Cc: davem@davemloft.net; kuba@kernel.org; frieder.schrempf@kontron.de;
>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
>> <linux-imx@nxp.com>
>> Subject: Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations
>>
>> On Thu, May 27, 2021 at 12:10:47PM +0000, Joakim Zhang wrote:
>>>
>>> Hi Frieder,
>>>
>>> As we talked before, could you please help test the patches when you are
>> free? Thanks.
>>
>> Hi Frieder
>>
>> If you can, could you also test it with traffic with a mixture of VLAN priorities.
>> You might want to force the link to 10Full, so you can overload it. Then see
>> what traffic actually makes it through.
> 
> Did your mailbox get bombed, let you miss this mail, hope you can see this reply.
> 
> Could you please give some feedback if it is possible? Thanks :-)

Thanks for the patches! As far as the bandwidth drops are concerned, this solves the problem. According to my simple iperf tests the untagged TX traffic now always goes to queue 0 and therefore doesn't see any random bandwidth limitations anymore. 

Regarding Andrew's request for testing this with some tagged traffic mix, my problem is that though I have used VLANs before and know how to set them up, I've never done anything with QoS, so I don't really now how to set the priorities (looks like I need to set up internal priorities and egress mapping somehow!?). If you have any pointers for this it would be appreciated. I probably could do some quick verification tests, but I don't have the time to really dive into the topic.

Best regards
Frieder

RE: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations

[Joakim Zhang]

Hi Frieder,

> -----Original Message-----
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> Sent: 2021年6月8日 22:19
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Andrew Lunn
> <andrew@lunn.ch>
> Cc: davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth fluctuations
> 
> Hi Joakim, hi Andrew,
> 
> On 08.06.21 05:23, Joakim Zhang wrote:
> >
> > Hi Frieder,
> >
> >> -----Original Message-----
> >> From: Andrew Lunn <andrew@lunn.ch>
> >> Sent: 2021年5月27日 22:06
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> >> Cc: davem@davemloft.net; kuba@kernel.org;
> >> frieder.schrempf@kontron.de; netdev@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH V1 net-next 0/2] net: fec: fix TX bandwidth
> >> fluctuations
> >>
> >> On Thu, May 27, 2021 at 12:10:47PM +0000, Joakim Zhang wrote:
> >>>
> >>> Hi Frieder,
> >>>
> >>> As we talked before, could you please help test the patches when you
> >>> are
> >> free? Thanks.
> >>
> >> Hi Frieder
> >>
> >> If you can, could you also test it with traffic with a mixture of VLAN
> priorities.
> >> You might want to force the link to 10Full, so you can overload it.
> >> Then see what traffic actually makes it through.
> >
> > Did your mailbox get bombed, let you miss this mail, hope you can see this
> reply.
> >
> > Could you please give some feedback if it is possible? Thanks :-)
> 
> Thanks for the patches! As far as the bandwidth drops are concerned, this
> solves the problem. According to my simple iperf tests the untagged TX traffic
> now always goes to queue 0 and therefore doesn't see any random bandwidth
> limitations anymore.
>
> Regarding Andrew's request for testing this with some tagged traffic mix, my
> problem is that though I have used VLANs before and know how to set them up,
> I've never done anything with QoS, so I don't really now how to set the
> priorities (looks like I need to set up internal priorities and egress mapping
> somehow!?). If you have any pointers for this it would be appreciated. I
> probably could do some quick verification tests, but I don't have the time to
> really dive into the topic.

Thanks a lot for your testing! Could I add your t-b and r-b tag when I repost the patches to change functions into static as Jakub commented before?

Best Regards,
Joakim Zhang
> 
> Best regards
> Frieder