| Message ID | 20230208003327.29538-1-muhammad.husaini.zulkifli@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] igc: offload queue max SDU from tc-taprio | expand |
On Wed, 8 Feb 2023 08:33:27 +0800 Muhammad Husaini Zulkifli wrote: > From: Tan Tee Min <tee.min.tan@linux.intel.com> > > Add support for configuring the max SDU for each Tx queue. > If not specified, keep the default. > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 0cc327294dfb5..38ad437957ada 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -1508,6 +1508,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > __le32 launch_time = 0; > u32 tx_flags = 0; > unsigned short f; > + u32 max_sdu = 0; This variable can be moved to the scope of the if() ? > ktime_t txtime; > u8 hdr_len = 0; > int tso = 0; > @@ -1527,6 +1528,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > return NETDEV_TX_BUSY; > } > > + if (tx_ring->max_sdu > 0) { > + max_sdu = tx_ring->max_sdu + > + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); > + > + if (skb->len > max_sdu) You should increment some counter here. Otherwise it's a silent discard. > + goto skb_drop; > + } > + > if (!tx_ring->launchtime_enable) > goto done; > > @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > dev_kfree_skb_any(first->skb); first->skb is skb, as far as I can tell, you can reshuffle this code to avoid adding the new return flow. > first->skb = NULL; > > + return NETDEV_TX_OK; > + > +skb_drop: > + dev_kfree_skb_any(skb); > + > return NETDEV_TX_OK; > } > @@ -6122,6 +6137,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > } > } > > + for (i = 0; i < adapter->num_tx_queues; i++) { > + struct igc_ring *ring = adapter->tx_ring[i]; > + struct net_device *dev = adapter->netdev; > + > + if (qopt->max_sdu[i]) > + ring->max_sdu = qopt->max_sdu[i] + dev->hard_header_len; why hard_header_len? Isn't it always ETH_HLEN? > + else > + ring->max_sdu = 0;
Hi Jakub, > -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Thursday, 9 February, 2023 1:30 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius > <vinicius.gomes@intel.com>; naamax.meir@linux.intel.com; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; leon@kernel.org; > davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; > tee.min.tan@linux.intel.com; netdev@vger.kernel.org; Neftin, Sasha > <sasha.neftin@intel.com> > Subject: Re: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio > > On Wed, 8 Feb 2023 08:33:27 +0800 Muhammad Husaini Zulkifli wrote: > > From: Tan Tee Min <tee.min.tan@linux.intel.com> > > > > Add support for configuring the max SDU for each Tx queue. > > If not specified, keep the default. > > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > b/drivers/net/ethernet/intel/igc/igc_main.c > > index 0cc327294dfb5..38ad437957ada 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -1508,6 +1508,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > __le32 launch_time = 0; > > u32 tx_flags = 0; > > unsigned short f; > > + u32 max_sdu = 0; > > This variable can be moved to the scope of the if() ? Sure. Let me move to the if() scope in v5. > > > ktime_t txtime; > > u8 hdr_len = 0; > > int tso = 0; > > @@ -1527,6 +1528,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > return NETDEV_TX_BUSY; > > } > > > > + if (tx_ring->max_sdu > 0) { > > + max_sdu = tx_ring->max_sdu + > > + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); > > + > > + if (skb->len > max_sdu) > > You should increment some counter here. Otherwise it's a silent discard. I am thinking to use tx_dropped counters for this. Is it ok? > > > + goto skb_drop; > > + } > > + > > if (!tx_ring->launchtime_enable) > > goto done; > > > > @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > dev_kfree_skb_any(first->skb); > > first->skb is skb, as far as I can tell, you can reshuffle this code to > avoid adding the new return flow. What we try to do is to check the current max_sdu size at the beginning stage of the func() and drop it quickly. > > > first->skb = NULL; > > > > + return NETDEV_TX_OK; > > + > > +skb_drop: > > + dev_kfree_skb_any(skb); > > + > > return NETDEV_TX_OK; > > } > > > @@ -6122,6 +6137,16 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > > } > > } > > > > + for (i = 0; i < adapter->num_tx_queues; i++) { > > + struct igc_ring *ring = adapter->tx_ring[i]; > > + struct net_device *dev = adapter->netdev; > > + > > + if (qopt->max_sdu[i]) > > + ring->max_sdu = qopt->max_sdu[i] + dev- > >hard_header_len; > > why hard_header_len? Isn't it always ETH_HLEN? We followed the taprio_parse_tc_entries() implementation for this. But the hard_header_len should be same value as ETH_HLEN. > > > + else > > + ring->max_sdu = 0;
On Tue, 14 Feb 2023 06:27:17 +0000 Zulkifli, Muhammad Husaini wrote: > > > + if (tx_ring->max_sdu > 0) { > > > + max_sdu = tx_ring->max_sdu + > > > + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); > > > + > > > + if (skb->len > max_sdu) > > > > You should increment some counter here. Otherwise it's a silent discard. > > I am thinking to use tx_dropped counters for this. Is it ok? Yup! > > > + goto skb_drop; > > > + } > > > + > > > if (!tx_ring->launchtime_enable) > > > goto done; > > > > > > @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > > > dev_kfree_skb_any(first->skb); > > > > first->skb is skb, as far as I can tell, you can reshuffle this code to > > avoid adding the new return flow. > > What we try to do is to check the current max_sdu size at the > beginning stage of the func() and drop it quickly. I understand, what I'm saying is that the code which is already here can be reused, it currently operates on first->skb but it can as well use skb. And then you'll be able to jump to the same statement, rather than have to create a separate return.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, 14 February, 2023 2:40 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Gomes, Vinicius <vinicius.gomes@intel.com>; > naamax.meir@linux.intel.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; leon@kernel.org; davem@davemloft.net; > pabeni@redhat.com; edumazet@google.com; tee.min.tan@linux.intel.com; > netdev@vger.kernel.org; Neftin, Sasha <sasha.neftin@intel.com> > Subject: Re: [PATCH net-next v3] igc: offload queue max SDU from tc-taprio > > On Tue, 14 Feb 2023 06:27:17 +0000 Zulkifli, Muhammad Husaini wrote: > > > > + if (tx_ring->max_sdu > 0) { > > > > + max_sdu = tx_ring->max_sdu + > > > > + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); > > > > + > > > > + if (skb->len > max_sdu) > > > > > > You should increment some counter here. Otherwise it's a silent discard. > > > > I am thinking to use tx_dropped counters for this. Is it ok? > > Yup! > > > > > + goto skb_drop; > > > > + } > > > > + > > > > if (!tx_ring->launchtime_enable) > > > > goto done; > > > > > > > > @@ -1606,6 +1615,11 @@ static netdev_tx_t > igc_xmit_frame_ring(struct sk_buff *skb, > > > > dev_kfree_skb_any(first->skb); > > > > > > first->skb is skb, as far as I can tell, you can reshuffle this code > > > first->to > > > avoid adding the new return flow. > > > > What we try to do is to check the current max_sdu size at the > > beginning stage of the func() and drop it quickly. > > I understand, what I'm saying is that the code which is already here can be > reused, it currently operates on first->skb but it can as well use skb. And then > you'll be able to jump to the same statement, rather than have to create a > separate return. Sure. Thanks
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 9db93c1f97679..34aebf00a5123 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -99,6 +99,7 @@ struct igc_ring { u32 start_time; u32 end_time; + u32 max_sdu; /* CBS parameters */ bool cbs_enable; /* indicates if CBS is enabled */ diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 0cc327294dfb5..38ad437957ada 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1508,6 +1508,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, __le32 launch_time = 0; u32 tx_flags = 0; unsigned short f; + u32 max_sdu = 0; ktime_t txtime; u8 hdr_len = 0; int tso = 0; @@ -1527,6 +1528,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, return NETDEV_TX_BUSY; } + if (tx_ring->max_sdu > 0) { + max_sdu = tx_ring->max_sdu + + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); + + if (skb->len > max_sdu) + goto skb_drop; + } + if (!tx_ring->launchtime_enable) goto done; @@ -1606,6 +1615,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, dev_kfree_skb_any(first->skb); first->skb = NULL; + return NETDEV_TX_OK; + +skb_drop: + dev_kfree_skb_any(skb); + return NETDEV_TX_OK; } @@ -6039,6 +6053,7 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) ring->start_time = 0; ring->end_time = NSEC_PER_SEC; + ring->max_sdu = 0; } return 0; @@ -6122,6 +6137,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, } } + for (i = 0; i < adapter->num_tx_queues; i++) { + struct igc_ring *ring = adapter->tx_ring[i]; + struct net_device *dev = adapter->netdev; + + if (qopt->max_sdu[i]) + ring->max_sdu = qopt->max_sdu[i] + dev->hard_header_len; + else + ring->max_sdu = 0; + } + return 0; } @@ -6221,6 +6246,7 @@ static int igc_tc_query_caps(struct igc_adapter *adapter, if (hw->mac.type != igc_i225) return -EOPNOTSUPP; + caps->supports_queue_max_sdu = true; caps->gate_mask_per_txq = true; return 0;