* [PATCH net-next v1] igc: offload queue max SDU from tc-taprio @ 2022-12-14 14:45 Muhammad Husaini Zulkifli 2022-12-14 17:17 ` Vinicius Costa Gomes 2023-01-01 11:13 ` naamax.meir 0 siblings, 2 replies; 5+ messages in thread From: Muhammad Husaini Zulkifli @ 2022-12-14 14:45 UTC (permalink / raw) To: intel-wired-lan, vinicius.gomes Cc: tee.min.tan, davem, kuba, netdev, muhammad.husaini.zulkifli, naamax.meir, anthony.l.nguyen 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. Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> --- drivers/net/ethernet/intel/igc/igc.h | 1 + drivers/net/ethernet/intel/igc/igc_main.c | 45 +++++++++++++++++++++++ include/net/pkt_sched.h | 1 + net/sched/sch_taprio.c | 4 +- 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 5da8d162cd38..ce9e88687d8c 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 e07287e05862..7ce05c31e371 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,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, return NETDEV_TX_BUSY; } + if (tx_ring->max_sdu > 0) { + if (skb_vlan_tagged(skb)) + max_sdu = tx_ring->max_sdu + VLAN_HLEN; + else + max_sdu = tx_ring->max_sdu; + + if (skb->len > max_sdu) + goto skb_drop; + } + if (!tx_ring->launchtime_enable) goto done; @@ -1606,6 +1617,12 @@ 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); + skb = NULL; + return NETDEV_TX_OK; } @@ -6015,6 +6032,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; @@ -6097,6 +6115,15 @@ 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]; + + if (qopt->max_frm_len[i] == U32_MAX) + ring->max_sdu = 0; + else + ring->max_sdu = qopt->max_frm_len[i]; + } + return 0; } @@ -6184,12 +6211,30 @@ static int igc_tsn_enable_cbs(struct igc_adapter *adapter, return igc_tsn_offload_apply(adapter); } +static int igc_tsn_query_caps(struct tc_query_caps_base *base) +{ + switch (base->type) { + case TC_SETUP_QDISC_TAPRIO: { + struct tc_taprio_caps *caps = base->caps; + + caps->supports_queue_max_sdu = true; + + return 0; + } + default: + return -EOPNOTSUPP; + } +} + static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data) { struct igc_adapter *adapter = netdev_priv(dev); switch (type) { + case TC_QUERY_CAPS: + return igc_tsn_query_caps(type_data); + case TC_SETUP_QDISC_TAPRIO: return igc_tsn_enable_qbv_scheduling(adapter, type_data); diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index 38207873eda6..d2539b1f6529 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -178,6 +178,7 @@ struct tc_taprio_qopt_offload { u64 cycle_time; u64 cycle_time_extension; u32 max_sdu[TC_MAX_QUEUE]; + u32 max_frm_len[TC_MAX_QUEUE]; size_t num_entries; struct tc_taprio_sched_entry entries[]; diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 570389f6cdd7..d39164074756 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -1263,8 +1263,10 @@ static int taprio_enable_offload(struct net_device *dev, offload->enable = 1; taprio_sched_to_offload(dev, sched, offload); - for (tc = 0; tc < TC_MAX_QUEUE; tc++) + for (tc = 0; tc < TC_MAX_QUEUE; tc++) { offload->max_sdu[tc] = q->max_sdu[tc]; + offload->max_frm_len[tc] = q->max_frm_len[tc]; + } err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload); if (err < 0) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio 2022-12-14 14:45 [PATCH net-next v1] igc: offload queue max SDU from tc-taprio Muhammad Husaini Zulkifli @ 2022-12-14 17:17 ` Vinicius Costa Gomes 2022-12-15 6:05 ` Zulkifli, Muhammad Husaini 2023-01-01 11:13 ` naamax.meir 1 sibling, 1 reply; 5+ messages in thread From: Vinicius Costa Gomes @ 2022-12-14 17:17 UTC (permalink / raw) To: Muhammad Husaini Zulkifli, intel-wired-lan Cc: tee.min.tan, davem, kuba, netdev, muhammad.husaini.zulkifli, naamax.meir, anthony.l.nguyen Hi, Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> writes: > 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. > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 1 + > drivers/net/ethernet/intel/igc/igc_main.c | 45 +++++++++++++++++++++++ > include/net/pkt_sched.h | 1 + > net/sched/sch_taprio.c | 4 +- > 4 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 5da8d162cd38..ce9e88687d8c 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 e07287e05862..7ce05c31e371 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,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > return NETDEV_TX_BUSY; > } > > + if (tx_ring->max_sdu > 0) { > + if (skb_vlan_tagged(skb)) > + max_sdu = tx_ring->max_sdu + VLAN_HLEN; > + else > + max_sdu = tx_ring->max_sdu; perhaps this? max_sdu = tx_ring->max_sdu + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); Totally optional. > + > + if (skb->len > max_sdu) > + goto skb_drop; > + } > + I don't think the overhead would be measurable for the pkt/s rates that a 2.5G link can handle. But a test and a note in the commit message confirming that would be nice. > if (!tx_ring->launchtime_enable) > goto done; > > @@ -1606,6 +1617,12 @@ 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); > + skb = NULL; > + > return NETDEV_TX_OK; > } > > @@ -6015,6 +6032,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; > @@ -6097,6 +6115,15 @@ 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]; > + > + if (qopt->max_frm_len[i] == U32_MAX) > + ring->max_sdu = 0; > + else > + ring->max_sdu = qopt->max_frm_len[i]; > + } > + > return 0; > } > > @@ -6184,12 +6211,30 @@ static int igc_tsn_enable_cbs(struct igc_adapter *adapter, > return igc_tsn_offload_apply(adapter); > } > > +static int igc_tsn_query_caps(struct tc_query_caps_base *base) > +{ > + switch (base->type) { > + case TC_SETUP_QDISC_TAPRIO: { > + struct tc_taprio_caps *caps = base->caps; > + > + caps->supports_queue_max_sdu = true; > + > + return 0; > + } > + default: > + return -EOPNOTSUPP; > + } > +} > + > static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, > void *type_data) > { > struct igc_adapter *adapter = netdev_priv(dev); > > switch (type) { > + case TC_QUERY_CAPS: > + return igc_tsn_query_caps(type_data); > + > case TC_SETUP_QDISC_TAPRIO: > return igc_tsn_enable_qbv_scheduling(adapter, type_data); > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h > index 38207873eda6..d2539b1f6529 100644 > --- a/include/net/pkt_sched.h > +++ b/include/net/pkt_sched.h > @@ -178,6 +178,7 @@ struct tc_taprio_qopt_offload { > u64 cycle_time; > u64 cycle_time_extension; > u32 max_sdu[TC_MAX_QUEUE]; > + u32 max_frm_len[TC_MAX_QUEUE]; > 'max_frm_len' is an internal taprio optimization, to simplify the code where the underlying HW doesn't support offload. For offloading, only 'max_sdu' should be used. Unless you have a strong reason. If you have that reason, it should be a separate commit. > size_t num_entries; > struct tc_taprio_sched_entry entries[]; > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > index 570389f6cdd7..d39164074756 100644 > --- a/net/sched/sch_taprio.c > +++ b/net/sched/sch_taprio.c > @@ -1263,8 +1263,10 @@ static int taprio_enable_offload(struct net_device *dev, > offload->enable = 1; > taprio_sched_to_offload(dev, sched, offload); > > - for (tc = 0; tc < TC_MAX_QUEUE; tc++) > + for (tc = 0; tc < TC_MAX_QUEUE; tc++) { > offload->max_sdu[tc] = q->max_sdu[tc]; > + offload->max_frm_len[tc] = q->max_frm_len[tc]; > + } > > err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload); > if (err < 0) { > -- > 2.17.1 > -- Vinicius ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio 2022-12-14 17:17 ` Vinicius Costa Gomes @ 2022-12-15 6:05 ` Zulkifli, Muhammad Husaini 2022-12-15 16:04 ` Vinicius Costa Gomes 0 siblings, 1 reply; 5+ messages in thread From: Zulkifli, Muhammad Husaini @ 2022-12-15 6:05 UTC (permalink / raw) To: Gomes, Vinicius, intel-wired-lan@osuosl.org Cc: tee.min.tan@linux.intel.com, davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org, naamax.meir@linux.intel.com, Nguyen, Anthony L Hi Vinicius, > -----Original Message----- > From: Gomes, Vinicius <vinicius.gomes@intel.com> > Sent: Thursday, 15 December, 2022 1:17 AM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; > intel-wired-lan@osuosl.org > Cc: tee.min.tan@linux.intel.com; davem@davemloft.net; kuba@kernel.org; > netdev@vger.kernel.org; Zulkifli, Muhammad Husaini > <muhammad.husaini.zulkifli@intel.com>; naamax.meir@linux.intel.com; > Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Subject: Re: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio > > Hi, > > Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> writes: > > > 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. > > > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > --- > > drivers/net/ethernet/intel/igc/igc.h | 1 + > > drivers/net/ethernet/intel/igc/igc_main.c | 45 > +++++++++++++++++++++++ > > include/net/pkt_sched.h | 1 + > > net/sched/sch_taprio.c | 4 +- > > 4 files changed, 50 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc.h > > b/drivers/net/ethernet/intel/igc/igc.h > > index 5da8d162cd38..ce9e88687d8c 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 e07287e05862..7ce05c31e371 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,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > return NETDEV_TX_BUSY; > > } > > > > + if (tx_ring->max_sdu > 0) { > > + if (skb_vlan_tagged(skb)) > > + max_sdu = tx_ring->max_sdu + VLAN_HLEN; > > + else > > + max_sdu = tx_ring->max_sdu; > > perhaps this? > max_sdu = tx_ring->max_sdu + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); > > Totally optional. Sure. We can change to above suggestion. > > > + > > + if (skb->len > max_sdu) > > + goto skb_drop; > > + } > > + > > I don't think the overhead would be measurable for the pkt/s rates that a > 2.5G link can handle. But a test and a note in the commit message confirming > that would be nice. IMHO, it should not depends on the link speed but the packet size only. If we detect packet size greater than max_sdu, we will just drop it. > > > if (!tx_ring->launchtime_enable) > > goto done; > > > > @@ -1606,6 +1617,12 @@ 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); > > + skb = NULL; > > + > > return NETDEV_TX_OK; > > } > > > > @@ -6015,6 +6032,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; > > @@ -6097,6 +6115,15 @@ 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]; > > + > > + if (qopt->max_frm_len[i] == U32_MAX) > > + ring->max_sdu = 0; > > + else > > + ring->max_sdu = qopt->max_frm_len[i]; > > + } > > + > > return 0; > > } > > > > @@ -6184,12 +6211,30 @@ static int igc_tsn_enable_cbs(struct igc_adapter > *adapter, > > return igc_tsn_offload_apply(adapter); } > > > > +static int igc_tsn_query_caps(struct tc_query_caps_base *base) { > > + switch (base->type) { > > + case TC_SETUP_QDISC_TAPRIO: { > > + struct tc_taprio_caps *caps = base->caps; > > + > > + caps->supports_queue_max_sdu = true; > > + > > + return 0; > > + } > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, > > void *type_data) > > { > > struct igc_adapter *adapter = netdev_priv(dev); > > > > switch (type) { > > + case TC_QUERY_CAPS: > > + return igc_tsn_query_caps(type_data); > > + > > case TC_SETUP_QDISC_TAPRIO: > > return igc_tsn_enable_qbv_scheduling(adapter, type_data); > > > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index > > 38207873eda6..d2539b1f6529 100644 > > --- a/include/net/pkt_sched.h > > +++ b/include/net/pkt_sched.h > > @@ -178,6 +178,7 @@ struct tc_taprio_qopt_offload { > > u64 cycle_time; > > u64 cycle_time_extension; > > u32 max_sdu[TC_MAX_QUEUE]; > > + u32 max_frm_len[TC_MAX_QUEUE]; > > > > 'max_frm_len' is an internal taprio optimization, to simplify the code where > the underlying HW doesn't support offload. The max_sdu only comes with MTU payload size. The reason why we are using this max_frm_len is to get the header + MTU size together. We can use max_sdu + header in the igc_save_qbv_schedule() and remove this piece of code from pkt_sched.h > > For offloading, only 'max_sdu' should be used. Unless you have a strong > reason. If you have that reason, it should be a separate commit. > > > size_t num_entries; > > struct tc_taprio_sched_entry entries[]; diff --git > > a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index > > 570389f6cdd7..d39164074756 100644 > > --- a/net/sched/sch_taprio.c > > +++ b/net/sched/sch_taprio.c > > @@ -1263,8 +1263,10 @@ static int taprio_enable_offload(struct > net_device *dev, > > offload->enable = 1; > > taprio_sched_to_offload(dev, sched, offload); > > > > - for (tc = 0; tc < TC_MAX_QUEUE; tc++) > > + for (tc = 0; tc < TC_MAX_QUEUE; tc++) { > > offload->max_sdu[tc] = q->max_sdu[tc]; > > + offload->max_frm_len[tc] = q->max_frm_len[tc]; > > + } > > > > err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload); > > if (err < 0) { > > -- > > 2.17.1 > > > > -- > Vinicius ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio 2022-12-15 6:05 ` Zulkifli, Muhammad Husaini @ 2022-12-15 16:04 ` Vinicius Costa Gomes 0 siblings, 0 replies; 5+ messages in thread From: Vinicius Costa Gomes @ 2022-12-15 16:04 UTC (permalink / raw) To: Zulkifli, Muhammad Husaini, intel-wired-lan@osuosl.org Cc: tee.min.tan@linux.intel.com, davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org, naamax.meir@linux.intel.com, Nguyen, Anthony L Hi Husaini, "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@intel.com> writes: > Hi Vinicius, > >> -----Original Message----- >> From: Gomes, Vinicius <vinicius.gomes@intel.com> >> Sent: Thursday, 15 December, 2022 1:17 AM >> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; >> intel-wired-lan@osuosl.org >> Cc: tee.min.tan@linux.intel.com; davem@davemloft.net; kuba@kernel.org; >> netdev@vger.kernel.org; Zulkifli, Muhammad Husaini >> <muhammad.husaini.zulkifli@intel.com>; naamax.meir@linux.intel.com; >> Nguyen, Anthony L <anthony.l.nguyen@intel.com> >> Subject: Re: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio >> >> Hi, >> >> Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> writes: >> >> > 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. >> > >> > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> >> > Signed-off-by: Muhammad Husaini Zulkifli >> > <muhammad.husaini.zulkifli@intel.com> >> > --- >> > drivers/net/ethernet/intel/igc/igc.h | 1 + >> > drivers/net/ethernet/intel/igc/igc_main.c | 45 >> +++++++++++++++++++++++ >> > include/net/pkt_sched.h | 1 + >> > net/sched/sch_taprio.c | 4 +- >> > 4 files changed, 50 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/net/ethernet/intel/igc/igc.h >> > b/drivers/net/ethernet/intel/igc/igc.h >> > index 5da8d162cd38..ce9e88687d8c 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 e07287e05862..7ce05c31e371 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,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct >> sk_buff *skb, >> > return NETDEV_TX_BUSY; >> > } >> > >> > + if (tx_ring->max_sdu > 0) { >> > + if (skb_vlan_tagged(skb)) >> > + max_sdu = tx_ring->max_sdu + VLAN_HLEN; >> > + else >> > + max_sdu = tx_ring->max_sdu; >> >> perhaps this? >> max_sdu = tx_ring->max_sdu + (skb_vlan_tagged(skb) ? VLAN_HLEN : 0); >> >> Totally optional. > > Sure. We can change to above suggestion. > >> >> > + >> > + if (skb->len > max_sdu) >> > + goto skb_drop; >> > + } >> > + >> >> I don't think the overhead would be measurable for the pkt/s rates that a >> 2.5G link can handle. But a test and a note in the commit message confirming >> that would be nice. > > IMHO, it should not depends on the link speed but the packet size only. > If we detect packet size greater than max_sdu, we will just drop it. > I was thinking more about the added conditional on the hot path, if it had some performance impact for the case when packets are not dropped. I really don't think there will be any, but it's nice to have some numbers to confirm that. >> >> > if (!tx_ring->launchtime_enable) >> > goto done; >> > >> > @@ -1606,6 +1617,12 @@ 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); >> > + skb = NULL; >> > + >> > return NETDEV_TX_OK; >> > } >> > >> > @@ -6015,6 +6032,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; >> > @@ -6097,6 +6115,15 @@ 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]; >> > + >> > + if (qopt->max_frm_len[i] == U32_MAX) >> > + ring->max_sdu = 0; >> > + else >> > + ring->max_sdu = qopt->max_frm_len[i]; >> > + } >> > + >> > return 0; >> > } >> > >> > @@ -6184,12 +6211,30 @@ static int igc_tsn_enable_cbs(struct igc_adapter >> *adapter, >> > return igc_tsn_offload_apply(adapter); } >> > >> > +static int igc_tsn_query_caps(struct tc_query_caps_base *base) { >> > + switch (base->type) { >> > + case TC_SETUP_QDISC_TAPRIO: { >> > + struct tc_taprio_caps *caps = base->caps; >> > + >> > + caps->supports_queue_max_sdu = true; >> > + >> > + return 0; >> > + } >> > + default: >> > + return -EOPNOTSUPP; >> > + } >> > +} >> > + >> > static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type, >> > void *type_data) >> > { >> > struct igc_adapter *adapter = netdev_priv(dev); >> > >> > switch (type) { >> > + case TC_QUERY_CAPS: >> > + return igc_tsn_query_caps(type_data); >> > + >> > case TC_SETUP_QDISC_TAPRIO: >> > return igc_tsn_enable_qbv_scheduling(adapter, type_data); >> > >> > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index >> > 38207873eda6..d2539b1f6529 100644 >> > --- a/include/net/pkt_sched.h >> > +++ b/include/net/pkt_sched.h >> > @@ -178,6 +178,7 @@ struct tc_taprio_qopt_offload { >> > u64 cycle_time; >> > u64 cycle_time_extension; >> > u32 max_sdu[TC_MAX_QUEUE]; >> > + u32 max_frm_len[TC_MAX_QUEUE]; >> > >> >> 'max_frm_len' is an internal taprio optimization, to simplify the code where >> the underlying HW doesn't support offload. > > The max_sdu only comes with MTU payload size. The reason why we are using > this max_frm_len is to get the header + MTU size together. > > We can use max_sdu + header in the igc_save_qbv_schedule() and remove > this piece of code from pkt_sched.h This sounds better, only exposing max_sdu to the drivers, even if it causes a bit of duplicated code. > >> >> For offloading, only 'max_sdu' should be used. Unless you have a strong >> reason. If you have that reason, it should be a separate commit. >> >> > size_t num_entries; >> > struct tc_taprio_sched_entry entries[]; diff --git >> > a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index >> > 570389f6cdd7..d39164074756 100644 >> > --- a/net/sched/sch_taprio.c >> > +++ b/net/sched/sch_taprio.c >> > @@ -1263,8 +1263,10 @@ static int taprio_enable_offload(struct >> net_device *dev, >> > offload->enable = 1; >> > taprio_sched_to_offload(dev, sched, offload); >> > >> > - for (tc = 0; tc < TC_MAX_QUEUE; tc++) >> > + for (tc = 0; tc < TC_MAX_QUEUE; tc++) { >> > offload->max_sdu[tc] = q->max_sdu[tc]; >> > + offload->max_frm_len[tc] = q->max_frm_len[tc]; >> > + } >> > >> > err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload); >> > if (err < 0) { >> > -- >> > 2.17.1 >> > >> >> -- >> Vinicius -- Vinicius ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] igc: offload queue max SDU from tc-taprio 2022-12-14 14:45 [PATCH net-next v1] igc: offload queue max SDU from tc-taprio Muhammad Husaini Zulkifli 2022-12-14 17:17 ` Vinicius Costa Gomes @ 2023-01-01 11:13 ` naamax.meir 1 sibling, 0 replies; 5+ messages in thread From: naamax.meir @ 2023-01-01 11:13 UTC (permalink / raw) To: Muhammad Husaini Zulkifli, intel-wired-lan, vinicius.gomes Cc: tee.min.tan, davem, kuba, netdev, anthony.l.nguyen On 12/14/2022 16:45, 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. > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 1 + > drivers/net/ethernet/intel/igc/igc_main.c | 45 +++++++++++++++++++++++ > include/net/pkt_sched.h | 1 + > net/sched/sch_taprio.c | 4 +- > 4 files changed, 50 insertions(+), 1 deletion(-) Tested-by: Naama Meir <naamax.meir@linux.intel.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-01 11:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-14 14:45 [PATCH net-next v1] igc: offload queue max SDU from tc-taprio Muhammad Husaini Zulkifli 2022-12-14 17:17 ` Vinicius Costa Gomes 2022-12-15 6:05 ` Zulkifli, Muhammad Husaini 2022-12-15 16:04 ` Vinicius Costa Gomes 2023-01-01 11:13 ` naamax.meir
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).