netdev - RE: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 7 Jul 2023 07:39:07 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Leon Romanovsky <leon@...nel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Neftin, Sasha" <sasha.neftin@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, Tan Tee Min <tee.min.tan@...ux.intel.com>,
	"Choong, Chwee Lin" <chwee.lin.choong@...el.com>, Naama Meir
	<naamax.meir@...ux.intel.com>
Subject: RE: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed



> -----Original Message-----
> From: Leon Romanovsky <leon@...nel.org>
> Sent: Friday, 7 July, 2023 12:41 AM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; davem@...emloft.net;
> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> netdev@...r.kernel.org; Neftin, Sasha <sasha.neftin@...el.com>;
> richardcochran@...il.com; Tan Tee Min <tee.min.tan@...ux.intel.com>;
> Choong, Chwee Lin <chwee.lin.choong@...el.com>; Naama Meir
> <naamax.meir@...ux.intel.com>
> Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed
> 
> On Thu, Jul 06, 2023 at 12:43:33PM +0000, Zulkifli, Muhammad Husaini wrote:
> > Dear Leon,
> >
> > Thanks for reviewing 😊
> > Replied inline.
> >
> > > -----Original Message-----
> > > From: Leon Romanovsky <leon@...nel.org>
> > > Sent: Thursday, 6 July, 2023 3:56 PM
> > > To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
> > > Cc: davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> > > edumazet@...gle.com; netdev@...r.kernel.org; Zulkifli, Muhammad
> > > Husaini <muhammad.husaini.zulkifli@...el.com>; Neftin, Sasha
> > > <sasha.neftin@...el.com>; richardcochran@...il.com; Tan Tee Min
> > > <tee.min.tan@...ux.intel.com>; Choong, Chwee Lin
> > > <chwee.lin.choong@...el.com>; Naama Meir
> > > <naamax.meir@...ux.intel.com>
> > > Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is
> > > closed
> > >
> > > On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> > > > From: Muhammad Husaini Zulkifli
> > > > <muhammad.husaini.zulkifli@...el.com>
> > > >
> > > > If a user schedules a Gate Control List (GCL) to close one of the
> > > > QBV gates while also transmitting a packet to that closed gate, TX
> > > > Hang will be happen. HW would not drop any packet when the gate is
> > > > closed and keep queuing up in HW TX FIFO until the gate is re-opened.
> > > > This patch implements the solution to drop the packet for the
> > > > closed gate.
> > > >
> > > > This patch will also reset the adapter to perform SW
> > > > initialization for each 1st Gate Control List (GCL) to avoid hang.
> > > > This is due to the HW design, where changing to TSN transmit mode
> > > > requires SW initialization. Intel Discrete I225/6 transmit mode
> > > > cannot be changed when in dynamic mode according to Software User
> > > > Manual Section 7.5.2.1. Subsequent Gate Control List (GCL)
> > > > operations will proceed without a reset, as they already are in TSN Mode.
> > > >
> > > > Step to reproduce:
> > > >
> > > > DUT:
> > > > 1) Configure GCL List with certain gate close.
> > > >
> > > > BASE=$(date +%s%N)
> > > > tc qdisc replace dev $IFACE parent root handle 100 taprio \
> > > >     num_tc 4 \
> > > >     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> > > >     queues 1@0 1@1 1@2 1@3 \
> > > >     base-time $BASE \
> > > >     sched-entry S 0x8 500000 \
> > > >     sched-entry S 0x4 500000 \
> > > >     flags 0x2
> > > >
> > > > 2) Transmit the packet to closed gate. You may use udp_tai
> > > > application to transmit UDP packet to any of the closed gate.
> > > >
> > > > ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> > > >
> > > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> > > > Co-developed-by: Tan Tee Min <tee.min.tan@...ux.intel.com>
> > > > Signed-off-by: Tan Tee Min <tee.min.tan@...ux.intel.com>
> > > > Tested-by: Chwee Lin Choong <chwee.lin.choong@...el.com>
> > > > Signed-off-by: Muhammad Husaini Zulkifli
> > > > <muhammad.husaini.zulkifli@...el.com>
> > > > Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
> > > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
> > > >  drivers/net/ethernet/intel/igc/igc_main.c | 58
> > > > +++++++++++++++++++++--  drivers/net/ethernet/intel/igc/igc_tsn.c
> > > > +++++++++++++++++++++|
> > > > 41 ++++++++++------
> > > >  3 files changed, 87 insertions(+), 18 deletions(-)
> 
> <...>
> 
> > > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct
> > > > +hrtimer
> > > > +*timer) {
> > > > +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> > > > +						   hrtimer);
> > > > +	unsigned int i;
> > > > +
> > > > +	adapter->qbv_transition = true;
> > > > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > > > +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> > > > +
> > > > +		if (tx_ring->admin_gate_closed) {
> > >
> > > Doesn't asynchronic access to shared variable through hrtimer
> > > require some sort of locking?
> >
> > Yeah I agreed with you. However, IMHO, it should be saved without the lock.
> > These variables, admin_gate_closed and oper_gate_closed, were set
> > during the transition and setup/delete of the TC only. The
> > qbv_transition flag has been used to protect the operation when it is in qbv
> transition.
> 
> I have no idea what last sentence means, but igc_qbv_scheduling_timer() and
> tx_ring are global function/variables and TC setup/delete can run in parallel to
> them.
> 

Thanks for pointing it out. I will add the lock here. 
Will send out new version. Testing it right now.

Thanks,
Husaini

> Thanks