| 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
| ||
|
Date: Thu, 6 Jul 2023 12:43:33 +0000
From: "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To: Leon Romanovsky <leon@...nel.org>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>
CC: "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
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(-)
>
> <...>
>
> > @@ -6149,6 +6157,8 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> > adapter->cycle_time = qopt->cycle_time;
> > adapter->base_time = qopt->base_time;
> >
> > + igc_ptp_read(adapter, &now);
> > +
> > for (n = 0; n < qopt->num_entries; n++) {
> > struct tc_taprio_sched_entry *e = &qopt->entries[n];
> >
> > @@ -6183,7 +6193,10 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> > ring->start_time = start_time;
> > ring->end_time = end_time;
> >
> > - queue_configured[i] = true;
> > + if (ring->start_time >= adapter->cycle_time)
> > + queue_configured[i] = false;
> > + else
> > + queue_configured[i] = true;
> > }
> >
> > start_time += e->interval;
> > @@ -6193,8 +6206,20 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> > * If not, set the start and end time to be end time.
> > */
> > for (i = 0; i < adapter->num_tx_queues; i++) {
> > + struct igc_ring *ring = adapter->tx_ring[i];
> > +
> > + if (!is_base_time_past(qopt->base_time, &now)) {
> > + ring->admin_gate_closed = false;
> > + } else {
> > + ring->oper_gate_closed = false;
> > + ring->admin_gate_closed = false;
> > + }
> > +
> > if (!queue_configured[i]) {
> > - struct igc_ring *ring = adapter->tx_ring[i];
> > + if (!is_base_time_past(qopt->base_time, &now))
> > + ring->admin_gate_closed = true;
> > + else
> > + ring->oper_gate_closed = true;
> >
> > ring->start_time = end_time;
> > ring->end_time = end_time;
> > @@ -6575,6 +6600,27 @@ static const struct xdp_metadata_ops
> igc_xdp_metadata_ops = {
> > .xmo_rx_timestamp = igc_xdp_rx_timestamp,
> > };
> >
> > +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.
Thanks,
Husaini
>
> Thanks
>
> > + tx_ring->admin_gate_closed = false;
> > + tx_ring->oper_gate_closed = true;
> > + } else {
> > + tx_ring->oper_gate_closed = false;
> > + }
> > + }
> > + adapter->qbv_transition = false;
> > + return HRTIMER_NORESTART;
> > +}
> > +
Powered by blists - more mailing lists