On Thu, 30 May 2019 00:21:39 +0000, Patel, Vedang wrote: > > On May 29, 2019, at 2:06 PM, Jakub Kicinski <jakub.kicin...@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.kicin...@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.kicin...@netronome.com> wrote: > >>>>> On Tue, 28 May 2019 10:46:44 -0700, Vedang Patel wrote: > >>>>>> From: Vinicius Costa Gomes <vinicius.go...@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.go...@intel.com> > >>>>>> Signed-off-by: Vedang Patel <vedang.pa...@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!
