| Message ID | 1559065608-27888-4-git-send-email-vedang.patel@intel.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | net/sched: Add txtime assist support for taprio | expand |
Thanks Jacub for they inputs. > On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: >> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> >> This adds the UAPI and the core bits necessary for userspace to >> request hardware offloading to be enabled. >> >> The future commits will enable hybrid or full offloading for taprio. This >> commit sets up the infrastructure to enable it via the netlink interface. >> >> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> Signed-off-by: Vedang Patel <vedang.patel@intel.com> > > Other qdiscs offload by default, this offload-level selection here is a > little bit inconsistent with that :( > There are 2 different offload modes introduced in this patch. 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. 2. Full offload mode: This mode is currently not supported by any network driver. The support for this will be coming soon. But, we can enable this mode by default. Also, from what Vinicius tells me, offload modes for cbs, etf and mqprio are also disabled by default. So, it will make more sense to keep it disabled to be consistent with other qdiscs similar to this one. >> @@ -731,6 +857,9 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, >> if (err < 0) >> return err; >> >> + if (tb[TCA_TAPRIO_ATTR_OFFLOAD_FLAGS]) >> + offload_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_OFFLOAD_FLAGS]); > > You should make sure user doesn't set unknown bits. Otherwise using > other bits will not be possible in the future. > Yes, I agree here, will include this in the next patchset. Thanks, Vedang >> new_admin = kzalloc(sizeof(*new_admin), GFP_KERNEL); >> if (!new_admin) { >> NL_SET_ERR_MSG(extack, "Not enough memory for a new schedule");
On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: > > On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: > >> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >> > >> This adds the UAPI and the core bits necessary for userspace to > >> request hardware offloading to be enabled. > >> > >> The future commits will enable hybrid or full offloading for taprio. This > >> commit sets up the infrastructure to enable it via the netlink interface. > >> > >> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >> Signed-off-by: Vedang Patel <vedang.patel@intel.com> > > > > Other qdiscs offload by default, this offload-level selection here is a > > little bit inconsistent with that :( > > > There are 2 different offload modes introduced in this patch. > > 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. Excellent, it looks like there will be driver patches necessary for this offload to function, also it seems your offload enable function still contains this after the series: /* FIXME: enable offloading */ Please only post offload infrastructure when fully fleshed out and with driver patches making use of it. > 2. Full offload mode: This mode is currently not supported by any network driver. The support for this will be coming soon. But, we can enable this mode by default. > > Also, from what Vinicius tells me, offload modes for cbs, etf and mqprio are also disabled by default. So, it will make more sense to keep it disabled to be consistent with other qdiscs similar to this one.
On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com<mailto:jakub.kicinski@netronome.com>> wrote: On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com<mailto:jakub.kicinski@netronome.com>> wrote: On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: From: Vinicius Costa Gomes <vinicius.gomes@intel.com<mailto:vinicius.gomes@intel.com>> This adds the UAPI and the core bits necessary for userspace to request hardware offloading to be enabled. The future commits will enable hybrid or full offloading for taprio. This commit sets up the infrastructure to enable it via the netlink interface. Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com<mailto:vinicius.gomes@intel.com>> Signed-off-by: Vedang Patel <vedang.patel@intel.com<mailto:vedang.patel@intel.com>> Other qdiscs offload by default, this offload-level selection here is a little bit inconsistent with that :( There are 2 different offload modes introduced in this patch. 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. Excellent, it looks like there will be driver patches necessary for this offload to function, also it seems your offload enable function still contains this after the series: /* FIXME: enable offloading */ Please only post offload infrastructure when fully fleshed out and with driver patches making use of it. The above comment refers to the full offload mode described below. It is not needed for txtime offload mode. Txtime offload mode is essentially setting the transmit time for each packet depending on what interval it is going to be transmitted instead of relying on the hrtimers to open gates and send packets. More details about why exactly this is done is mentioned in patch #5[1] of this series. What we can do is just add the txtime offload related flag and add the full offload flag whenever the driver bits are ready. Does that address your concern? Thanks, Vedang [1] - https://lore.kernel.org/netdev/1559065608-27888-1-git-send-email-vedang.patel@intel.com/T/#mcbcd70a02eb3aea0622bf2a41e492398353b6923 2. Full offload mode: This mode is currently not supported by any network driver. The support for this will be coming soon. But, we can enable this mode by default. Also, from what Vinicius tells me, offload modes for cbs, etf and mqprio are also disabled by default. So, it will make more sense to keep it disabled to be consistent with other qdiscs similar to this one.
[Sending the email again since the last one was rejected by netdev because it was html.] > On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: >>> On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: >>> On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: >>>> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>> >>>> This adds the UAPI and the core bits necessary for userspace to >>>> request hardware offloading to be enabled. >>>> >>>> The future commits will enable hybrid or full offloading for taprio. This >>>> commit sets up the infrastructure to enable it via the netlink interface. >>>> >>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>> Signed-off-by: Vedang Patel <vedang.patel@intel.com> >>> >>> Other qdiscs offload by default, this offload-level selection here is a >>> little bit inconsistent with that :( >>> >> There are 2 different offload modes introduced in this patch. >> >> 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. > > Excellent, it looks like there will be driver patches necessary for > this offload to function, also it seems your offload enable function > still contains this after the series: > > /* FIXME: enable offloading */ > > Please only post offload infrastructure when fully fleshed out and with > driver patches making use of it. > The above comment refers to the full offload mode described below. It is not needed for txtime offload mode. Txtime offload mode is essentially setting the transmit time for each packet depending on what interval it is going to be transmitted instead of relying on the hrtimers to open gates and send packets. More details about why exactly this is done is mentioned in patch #5[1] of this series. What we can do is just add the txtime offload related flag and add the full offload flag whenever the driver bits are ready. Does that address your concern? Thanks, Vedang >> 2. Full offload mode: This mode is currently not supported by any network driver. The support for this will be coming soon. But, we can enable this mode by default. >> >> Also, from what Vinicius tells me, offload modes for cbs, etf and mqprio are also disabled by default. So, it will make more sense to keep it disabled to be consistent with other qdiscs similar to this one.
On Wed, 29 May 2019 20:05:16 +0000, Patel, Vedang wrote: > [Sending the email again since the last one was rejected by netdev because it was html.] > > > On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > > > On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: > >>> On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > >>> On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: > >>>> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >>>> > >>>> This adds the UAPI and the core bits necessary for userspace to > >>>> request hardware offloading to be enabled. > >>>> > >>>> The future commits will enable hybrid or full offloading for taprio. This > >>>> commit sets up the infrastructure to enable it via the netlink interface. > >>>> > >>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >>>> Signed-off-by: Vedang Patel <vedang.patel@intel.com> > >>> > >>> Other qdiscs offload by default, this offload-level selection here is a > >>> little bit inconsistent with that :( > >>> > >> There are 2 different offload modes introduced in this patch. > >> > >> 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. > > > > Excellent, it looks like there will be driver patches necessary for > > this offload to function, also it seems your offload enable function > > still contains this after the series: > > > > /* FIXME: enable offloading */ > > > > Please only post offload infrastructure when fully fleshed out and with > > driver patches making use of it. > > The above comment refers to the full offload mode described below. It > is not needed for txtime offload mode. Txtime offload mode is > essentially setting the transmit time for each packet depending on > what interval it is going to be transmitted instead of relying on the > hrtimers to open gates and send packets. More details about why > exactly this is done is mentioned in patch #5[1] of this series. From looking at this set it looks like I can add that qdisc to any netdev now *with* offload, and as long as the driver has _any_ ndo_setup_tc implementation taprio will not return an error. Perhaps this mode of operation should not be called offload? Can't the ETF qdisc underneath run in software mode? > What we can do is just add the txtime offload related flag and add > the full offload flag whenever the driver bits are ready. Does that > address your concern? That would be a step in the right direction. And please remove all the other unused code from the series. AFAICT this is what the enable offload function looks like after your set - what's the point of the 'err' variable? +static int taprio_enable_offload(struct net_device *dev, + struct tc_mqprio_qopt *mqprio, + struct taprio_sched *q, + struct sched_gate_list *sched, + struct netlink_ext_ack *extack, + u32 offload_flags) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err = 0; + + if (TXTIME_OFFLOAD_IS_ON(offload_flags)) + goto done; + + if (!FULL_OFFLOAD_IS_ON(offload_flags)) { + NL_SET_ERR_MSG(extack, "Offload mode is not supported"); + return -EOPNOTSUPP; + } + + if (!ops->ndo_setup_tc) { + NL_SET_ERR_MSG(extack, "Specified device does not support taprio offload"); + return -EOPNOTSUPP; + } + + /* FIXME: enable offloading */ + +done: + if (err == 0) { + q->dequeue = taprio_dequeue_offload; + q->peek = taprio_peek_offload; + q->offload_flags = offload_flags; + } + + return err; +}
On May 29, 2019, at 2:06 PM, Jakub Kicinski <jakub.kicinski@netronome.com<mailto:jakub.kicinski@netronome.com>> wrote: On Wed, 29 May 2019 20:05:16 +0000, Patel, Vedang wrote: [Sending the email again since the last one was rejected by netdev because it was html.] On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com<mailto:jakub.kicinski@netronome.com>> wrote: On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com<mailto:jakub.kicinski@netronome.com>> wrote: On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: From: Vinicius Costa Gomes <vinicius.gomes@intel.com<mailto:vinicius.gomes@intel.com>> This adds the UAPI and the core bits necessary for userspace to request hardware offloading to be enabled. The future commits will enable hybrid or full offloading for taprio. This commit sets up the infrastructure to enable it via the netlink interface. Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com<mailto:vinicius.gomes@intel.com>> Signed-off-by: Vedang Patel <vedang.patel@intel.com<mailto:vedang.patel@intel.com>> Other qdiscs offload by default, this offload-level selection here is a little bit inconsistent with that :( There are 2 different offload modes introduced in this patch. 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. Excellent, it looks like there will be driver patches necessary for this offload to function, also it seems your offload enable function still contains this after the series: /* FIXME: enable offloading */ Please only post offload infrastructure when fully fleshed out and with driver patches making use of it. The above comment refers to the full offload mode described below. It is not needed for txtime offload mode. Txtime offload mode is essentially setting the transmit time for each packet depending on what interval it is going to be transmitted instead of relying on the hrtimers to open gates and send packets. More details about why exactly this is done is mentioned in patch #5[1] of this series. From looking at this set it looks like I can add that qdisc to any netdev now *with* offload, and as long as the driver has _any_ ndo_setup_tc implementation taprio will not return an error. Perhaps this mode of operation should not be called offload? Can't the ETF qdisc underneath run in software mode? Yeah, it doesn’t make much sense to run ETF in software mode but it can be done. What about naming it txtime-assist instead? What we can do is just add the txtime offload related flag and add the full offload flag whenever the driver bits are ready. Does that address your concern? That would be a step in the right direction. And please remove all the other unused code from the series. AFAICT this is what the enable offload function looks like after your set - what's the point of the 'err' variable? Yes. This patch seems to have a few really silly mistakes. I apologize for that. I willl clean it up and send another version of the series. There is no unused code anywhere else in the series. +static int taprio_enable_offload(struct net_device *dev, + struct tc_mqprio_qopt *mqprio, + struct taprio_sched *q, + struct sched_gate_list *sched, + struct netlink_ext_ack *extack, + u32 offload_flags) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err = 0; + + if (TXTIME_OFFLOAD_IS_ON(offload_flags)) + goto done; + + if (!FULL_OFFLOAD_IS_ON(offload_flags)) { + NL_SET_ERR_MSG(extack, "Offload mode is not supported"); + return -EOPNOTSUPP; + } + + if (!ops->ndo_setup_tc) { + NL_SET_ERR_MSG(extack, "Specified device does not support taprio offload"); + return -EOPNOTSUPP; + } + + /* FIXME: enable offloading */ + +done: + if (err == 0) { + q->dequeue = taprio_dequeue_offload; + q->peek = taprio_peek_offload; + q->offload_flags = offload_flags; + } + + return err; +}
> On May 29, 2019, at 2:06 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 29 May 2019 20:05:16 +0000, Patel, Vedang wrote: >> [Sending the email again since the last one was rejected by netdev because it was html.] >> >>> On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: >>> >>> On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: >>>>> On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: >>>>> On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: >>>>>> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>>>> >>>>>> This adds the UAPI and the core bits necessary for userspace to >>>>>> request hardware offloading to be enabled. >>>>>> >>>>>> The future commits will enable hybrid or full offloading for taprio. This >>>>>> commit sets up the infrastructure to enable it via the netlink interface. >>>>>> >>>>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>>>> Signed-off-by: Vedang Patel <vedang.patel@intel.com> >>>>> >>>>> Other qdiscs offload by default, this offload-level selection here is a >>>>> little bit inconsistent with that :( >>>>> >>>> There are 2 different offload modes introduced in this patch. >>>> >>>> 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. >>> >>> Excellent, it looks like there will be driver patches necessary for >>> this offload to function, also it seems your offload enable function >>> still contains this after the series: >>> >>> /* FIXME: enable offloading */ >>> >>> Please only post offload infrastructure when fully fleshed out and with >>> driver patches making use of it. >> >> The above comment refers to the full offload mode described below. It >> is not needed for txtime offload mode. Txtime offload mode is >> essentially setting the transmit time for each packet depending on >> what interval it is going to be transmitted instead of relying on the >> hrtimers to open gates and send packets. More details about why >> exactly this is done is mentioned in patch #5[1] of this series. > > From looking at this set it looks like I can add that qdisc to any > netdev now *with* offload, and as long as the driver has _any_ > ndo_setup_tc implementation taprio will not return an error. > > Perhaps this mode of operation should not be called offload? Can't > the ETF qdisc underneath run in software mode? > Yeah, it doesn’t make much sense to run ETF in software mode but it can be done. What about naming it txtime-assist instead? >> What we can do is just add the txtime offload related flag and add >> the full offload flag whenever the driver bits are ready. Does that >> address your concern? > > That would be a step in the right direction. And please remove all the > other unused code from the series. AFAICT this is what the enable > offload function looks like after your set - what's the point of the > 'err' variable? > Yes. This patch seems to have a few really silly mistakes. I apologize for that. I willl clean it up and send another version of the series. There is no unused code anywhere else in the series. > +static int taprio_enable_offload(struct net_device *dev, > + struct tc_mqprio_qopt *mqprio, > + struct taprio_sched *q, > + struct sched_gate_list *sched, > + struct netlink_ext_ack *extack, > + u32 offload_flags) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + int err = 0; > + > + if (TXTIME_OFFLOAD_IS_ON(offload_flags)) > + goto done; > + > + if (!FULL_OFFLOAD_IS_ON(offload_flags)) { > + NL_SET_ERR_MSG(extack, "Offload mode is not supported"); > + return -EOPNOTSUPP; > + } > + > + if (!ops->ndo_setup_tc) { > + NL_SET_ERR_MSG(extack, "Specified device does not support taprio offload"); > + return -EOPNOTSUPP; > + } > + > + /* FIXME: enable offloading */ > + > +done: > + if (err == 0) { > + q->dequeue = taprio_dequeue_offload; > + q->peek = taprio_peek_offload; > + q->offload_flags = offload_flags; > + } > + > + return err; > +}
On Thu, 30 May 2019 00:21:39 +0000, Patel, Vedang wrote: > > On May 29, 2019, at 2:06 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 29 May 2019 20:05:16 +0000, Patel, Vedang wrote: > >> [Sending the email again since the last one was rejected by netdev because it was html.] > >> > >>> On May 29, 2019, at 12:14 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > >>> > >>> On Wed, 29 May 2019 17:06:49 +0000, Patel, Vedang wrote: > >>>>> On May 28, 2019, at 3:45 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > >>>>> On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: > >>>>>> From: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >>>>>> > >>>>>> This adds the UAPI and the core bits necessary for userspace to > >>>>>> request hardware offloading to be enabled. > >>>>>> > >>>>>> The future commits will enable hybrid or full offloading for taprio. This > >>>>>> commit sets up the infrastructure to enable it via the netlink interface. > >>>>>> > >>>>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > >>>>>> Signed-off-by: Vedang Patel <vedang.patel@intel.com> > >>>>> > >>>>> Other qdiscs offload by default, this offload-level selection here is a > >>>>> little bit inconsistent with that :( > >>>>> > >>>> There are 2 different offload modes introduced in this patch. > >>>> > >>>> 1. Txtime offload mode: This mode depends on skip_sock_check flag being set in the etf qdisc. Also, it requires some manual configuration which might be specific to the network adapter card. For example, for the i210 card, the user will have to route all the traffic to the highest priority queue and install etf qdisc with offload enabled on that queue. So, I don’t think this mode should be enabled by default. > >>> > >>> Excellent, it looks like there will be driver patches necessary for > >>> this offload to function, also it seems your offload enable function > >>> still contains this after the series: > >>> > >>> /* FIXME: enable offloading */ > >>> > >>> Please only post offload infrastructure when fully fleshed out and with > >>> driver patches making use of it. > >> > >> The above comment refers to the full offload mode described below. It > >> is not needed for txtime offload mode. Txtime offload mode is > >> essentially setting the transmit time for each packet depending on > >> what interval it is going to be transmitted instead of relying on the > >> hrtimers to open gates and send packets. More details about why > >> exactly this is done is mentioned in patch #5[1] of this series. > > > > From looking at this set it looks like I can add that qdisc to any > > netdev now *with* offload, and as long as the driver has _any_ > > ndo_setup_tc implementation taprio will not return an error. > > > > Perhaps this mode of operation should not be called offload? Can't > > the ETF qdisc underneath run in software mode? > > > Yeah, it doesn’t make much sense to run ETF in software mode but it > can be done. What about naming it txtime-assist instead? Sounds good! txtime.* works for me. > >> What we can do is just add the txtime offload related flag and add > >> the full offload flag whenever the driver bits are ready. Does that > >> address your concern? > > > > That would be a step in the right direction. And please remove all the > > other unused code from the series. AFAICT this is what the enable > > offload function looks like after your set - what's the point of the > > 'err' variable? > > > Yes. This patch seems to have a few really silly mistakes. I > apologize for that. I willl clean it up and send another version of > the series. There is no unused code anywhere else in the series. Thanks!
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h index 69fc52e4d6bd..3319255ffa25 100644 --- a/include/uapi/linux/pkt_sched.h +++ b/include/uapi/linux/pkt_sched.h @@ -1159,6 +1159,9 @@ enum { * [TCA_TAPRIO_ATTR_SCHED_ENTRY_INTERVAL] */ +#define TCA_TAPRIO_ATTR_OFFLOAD_FLAG_FULL_OFFLOAD 0x1 +#define TCA_TAPRIO_ATTR_OFFLOAD_FLAG_TXTIME_OFFLOAD 0x2 + enum { TCA_TAPRIO_ATTR_UNSPEC, TCA_TAPRIO_ATTR_PRIOMAP, /* struct tc_mqprio_qopt */ @@ -1170,6 +1173,7 @@ enum { TCA_TAPRIO_ATTR_ADMIN_SCHED, /* The admin sched, only used in dump */ TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME, /* s64 */ TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION, /* s64 */ + TCA_TAPRIO_ATTR_OFFLOAD_FLAGS, /* u32 */ __TCA_TAPRIO_ATTR_MAX, }; diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 9ecfb8f5902a..cee6b7a3dd85 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -26,6 +26,11 @@ static LIST_HEAD(taprio_list); static DEFINE_SPINLOCK(taprio_list_lock); #define TAPRIO_ALL_GATES_OPEN -1 +#define VALID_OFFLOAD(flags) ((flags) != U32_MAX) +#define FULL_OFFLOAD_IS_ON(flags) (VALID_OFFLOAD((flags)) && \ + ((flags) & TCA_TAPRIO_ATTR_OFFLOAD_FLAG_FULL_OFFLOAD)) +#define TXTIME_OFFLOAD_IS_ON(flags) (VALID_OFFLOAD((flags)) && \ + ((flags) & TCA_TAPRIO_ATTR_OFFLOAD_FLAG_TXTIME_OFFLOAD)) struct sched_entry { struct list_head list; @@ -55,6 +60,7 @@ struct sched_gate_list { struct taprio_sched { struct Qdisc **qdiscs; struct Qdisc *root; + u32 offload_flags; int clockid; atomic64_t picos_per_byte; /* Using picoseconds because for 10Gbps+ * speeds it's sub-nanoseconds per byte @@ -66,6 +72,8 @@ struct taprio_sched { struct sched_gate_list __rcu *oper_sched; struct sched_gate_list __rcu *admin_sched; ktime_t (*get_time)(void); + struct sk_buff *(*dequeue)(struct Qdisc *sch); + struct sk_buff *(*peek)(struct Qdisc *sch); struct hrtimer advance_timer; struct list_head taprio_list; }; @@ -143,7 +151,30 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch, return qdisc_enqueue(skb, child, to_free); } -static struct sk_buff *taprio_peek(struct Qdisc *sch) +static struct sk_buff *taprio_peek_offload(struct Qdisc *sch) +{ + struct taprio_sched *q = qdisc_priv(sch); + struct net_device *dev = qdisc_dev(sch); + struct sk_buff *skb; + int i; + + for (i = 0; i < dev->num_tx_queues; i++) { + struct Qdisc *child = q->qdiscs[i]; + + if (unlikely(!child)) + continue; + + skb = child->ops->peek(child); + if (!skb) + continue; + + return skb; + } + + return NULL; +} + +static struct sk_buff *taprio_peek_soft(struct Qdisc *sch) { struct taprio_sched *q = qdisc_priv(sch); struct net_device *dev = qdisc_dev(sch); @@ -184,6 +215,13 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch) return NULL; } +static struct sk_buff *taprio_peek(struct Qdisc *sch) +{ + struct taprio_sched *q = qdisc_priv(sch); + + return q->peek(sch); +} + static inline int length_to_duration(struct taprio_sched *q, int len) { return div_u64(len * atomic64_read(&q->picos_per_byte), 1000); @@ -196,7 +234,7 @@ static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry) atomic64_read(&q->picos_per_byte))); } -static struct sk_buff *taprio_dequeue(struct Qdisc *sch) +static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch) { struct taprio_sched *q = qdisc_priv(sch); struct net_device *dev = qdisc_dev(sch); @@ -275,6 +313,40 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch) return skb; } +static struct sk_buff *taprio_dequeue_offload(struct Qdisc *sch) +{ + struct taprio_sched *q = qdisc_priv(sch); + struct net_device *dev = qdisc_dev(sch); + struct sk_buff *skb; + int i; + + for (i = 0; i < dev->num_tx_queues; i++) { + struct Qdisc *child = q->qdiscs[i]; + + if (unlikely(!child)) + continue; + + skb = child->ops->dequeue(child); + if (unlikely(!skb)) + continue; + + qdisc_bstats_update(sch, skb); + qdisc_qstats_backlog_dec(sch, skb); + sch->q.qlen--; + + return skb; + } + + return NULL; +} + +static struct sk_buff *taprio_dequeue(struct Qdisc *sch) +{ + struct taprio_sched *q = qdisc_priv(sch); + + return q->dequeue(sch); +} + static bool should_restart_cycle(const struct sched_gate_list *oper, const struct sched_entry *entry) { @@ -707,6 +779,59 @@ static int taprio_dev_notifier(struct notifier_block *nb, unsigned long event, return NOTIFY_DONE; } +static void taprio_disable_offload(struct net_device *dev, + struct taprio_sched *q) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if (!q->offload_flags) + return; + + if (!ops->ndo_setup_tc) + return; + + /* FIXME: disable offloading */ + + /* Just to be sure to keep the function pointers in a + * consistent state always. + */ + q->dequeue = taprio_dequeue_soft; + q->peek = taprio_peek_soft; + + q->offload_flags = 0; +} + +static int taprio_enable_offload(struct net_device *dev, + struct tc_mqprio_qopt *mqprio, + struct taprio_sched *q, + struct sched_gate_list *sched, + struct netlink_ext_ack *extack, + u32 offload_flags) +{ + const struct net_device_ops *ops = dev->netdev_ops; + int err = 0; + + if (!FULL_OFFLOAD_IS_ON(offload_flags)) { + NL_SET_ERR_MSG(extack, "Offload mode is not supported"); + return -EOPNOTSUPP; + } + + if (!ops->ndo_setup_tc) { + NL_SET_ERR_MSG(extack, "Specified device does not support taprio offload"); + return -EOPNOTSUPP; + } + + /* FIXME: enable offloading */ + + q->dequeue = taprio_dequeue_offload; + q->peek = taprio_peek_offload; + + if (err == 0) + q->offload_flags = offload_flags; + + return err; +} + static int taprio_change(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack) { @@ -715,6 +840,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, struct taprio_sched *q = qdisc_priv(sch); struct net_device *dev = qdisc_dev(sch); struct tc_mqprio_qopt *mqprio = NULL; + u32 offload_flags = U32_MAX; int i, err, clockid; unsigned long flags; ktime_t start; @@ -731,6 +857,9 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, if (err < 0) return err; + if (tb[TCA_TAPRIO_ATTR_OFFLOAD_FLAGS]) + offload_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_OFFLOAD_FLAGS]); + new_admin = kzalloc(sizeof(*new_admin), GFP_KERNEL); if (!new_admin) { NL_SET_ERR_MSG(extack, "Not enough memory for a new schedule"); @@ -749,6 +878,12 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, goto free_sched; } + if (offload_flags != U32_MAX && (oper || admin)) { + NL_SET_ERR_MSG(extack, "Changing 'offload' of a running schedule is not supported"); + err = -ENOTSUPP; + goto free_sched; + } + err = parse_taprio_schedule(tb, new_admin, extack); if (err < 0) goto free_sched; @@ -823,6 +958,15 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt, goto unlock; } + if (!offload_flags) { + taprio_disable_offload(dev, q); + } else if (VALID_OFFLOAD(offload_flags) || q->offload_flags) { + err = taprio_enable_offload(dev, mqprio, q, + new_admin, extack, offload_flags); + if (err) + goto unlock; + } + err = taprio_get_start_time(sch, new_admin, &start); if (err < 0) { NL_SET_ERR_MSG(extack, "Internal error: failed get start time"); @@ -866,6 +1010,8 @@ static void taprio_destroy(struct Qdisc *sch) hrtimer_cancel(&q->advance_timer); + taprio_disable_offload(dev, q); + if (q->qdiscs) { for (i = 0; i < dev->num_tx_queues && q->qdiscs[i]; i++) qdisc_put(q->qdiscs[i]); @@ -895,6 +1041,9 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt, hrtimer_init(&q->advance_timer, CLOCK_TAI, HRTIMER_MODE_ABS); q->advance_timer.function = advance_sched; + q->dequeue = taprio_dequeue_soft; + q->peek = taprio_peek_soft; + q->root = sch; /* We only support static clockids. Use an invalid value as default @@ -1080,6 +1229,9 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb) if (nla_put_s32(skb, TCA_TAPRIO_ATTR_SCHED_CLOCKID, q->clockid)) goto options_error; + if (nla_put_u32(skb, TCA_TAPRIO_ATTR_OFFLOAD_FLAGS, q->offload_flags)) + goto options_error; + if (oper && dump_schedule(skb, oper)) goto options_error;