From: Vladimir Oltean <vladimir.oltean@nxp.com> To: Vinicius Costa Gomes <vinicius.gomes@intel.com> Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "jhs@mojatatu.com" <jhs@mojatatu.com>, "xiyou.wangcong@gmail.com" <xiyou.wangcong@gmail.com>, "jiri@resnulli.us" <jiri@resnulli.us>, "kuba@kernel.org" <kuba@kernel.org>, "Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>, Po Liu <po.liu@nxp.com>, "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>, "anthony.l.nguyen@intel.com" <anthony.l.nguyen@intel.com>, "mkubecek@suse.cz" <mkubecek@suse.cz> Subject: Re: [PATCH net-next v3 6/8] igc: Add support for tuning frame preemption via ethtool Date: Tue, 26 Jan 2021 00:32:44 +0000 [thread overview] Message-ID: <20210126003243.x3c44pmxmieqsa6e@skbuf> (raw) In-Reply-To: <20210122224453.4161729-7-vinicius.gomes@intel.com> On Fri, Jan 22, 2021 at 02:44:51PM -0800, Vinicius Costa Gomes wrote: > The tc subsystem sets which queues are marked as preemptible, it's the > role of ethtool to control more hardware specific parameters. These > parameters include: > > - enabling the frame preemption hardware: As enabling frame > preemption may have other requirements before it can be enabled, it's > exposed via the ethtool API; > > - mininum fragment size multiplier: expressed in usually in the form > of (1 + N)*64, this number indicates what's the size of the minimum > fragment that can be preempted. And not one word has been said about the patch... > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 12 +++++ > drivers/net/ethernet/intel/igc/igc_defines.h | 4 ++ > drivers/net/ethernet/intel/igc/igc_ethtool.c | 53 ++++++++++++++++++++ > drivers/net/ethernet/intel/igc/igc_tsn.c | 25 +++++++-- > 4 files changed, 91 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 35baae900c1f..1067c46e0bc2 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -87,6 +87,7 @@ struct igc_ring { > u8 queue_index; /* logical index of the ring*/ > u8 reg_idx; /* physical index of the ring */ > bool launchtime_enable; /* true if LaunchTime is enabled */ > + bool preemptible; /* true if not express */ Mixing tabs and spaces? > +static int igc_ethtool_set_preempt(struct net_device *netdev, > + struct ethtool_fp *fpcmd, > + struct netlink_ext_ack *extack) > +{ > + struct igc_adapter *adapter = netdev_priv(netdev); > + int i; > + > + if (fpcmd->add_frag_size < 68 || fpcmd->add_frag_size > 260) { > + NL_SET_ERR_MSG_MOD(extack, "Invalid value for add-frag-size"); > + return -EINVAL; > + } This check should belong in ethtool, since there's nothing unusual about this supported range. Also, I believe that Jakub requested the min-frag-size to be passed as 0, 1, 2, 3 as the standard specifies it, and not its multiplied version? > + > + adapter->frame_preemption_active = fpcmd->enabled; > + adapter->add_frag_size = fpcmd->add_frag_size; > + > + if (!adapter->frame_preemption_active) > + goto done; > + > + /* Enabling frame preemption requires TSN mode to be enabled, > + * which requires a schedule to be active. So, if there isn't > + * a schedule already configured, configure a simple one, with > + * all queues open, with 1ms cycle time. > + */ > + if (adapter->base_time) > + goto done; Unless I'm missing something, you are interpreting an adapter->base_time value of zero as "no Qbv schedule on port", as if it was invalid to have a base-time of zero, which it isn't. > @@ -115,6 +130,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > if (ring->launchtime_enable) > txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; > > + if (ring->preemptible) > + txqctl |= IGC_TXQCTL_PREEMPTABLE; I think this is the only place in the series where you use PREEMPTABLE instead of PREEMPTIBLE. > + > wr32(IGC_TXQCTL(i), txqctl); > } Out of curiosity, where is the ring to traffic class mapping configured in the igc driver? I suppose that you have more rings than traffic classes.
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Oltean <vladimir.oltean@nxp.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH net-next v3 6/8] igc: Add support for tuning frame preemption via ethtool Date: Tue, 26 Jan 2021 00:32:44 +0000 [thread overview] Message-ID: <20210126003243.x3c44pmxmieqsa6e@skbuf> (raw) In-Reply-To: <20210122224453.4161729-7-vinicius.gomes@intel.com> On Fri, Jan 22, 2021 at 02:44:51PM -0800, Vinicius Costa Gomes wrote: > The tc subsystem sets which queues are marked as preemptible, it's the > role of ethtool to control more hardware specific parameters. These > parameters include: > > - enabling the frame preemption hardware: As enabling frame > preemption may have other requirements before it can be enabled, it's > exposed via the ethtool API; > > - mininum fragment size multiplier: expressed in usually in the form > of (1 + N)*64, this number indicates what's the size of the minimum > fragment that can be preempted. And not one word has been said about the patch... > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc.h | 12 +++++ > drivers/net/ethernet/intel/igc/igc_defines.h | 4 ++ > drivers/net/ethernet/intel/igc/igc_ethtool.c | 53 ++++++++++++++++++++ > drivers/net/ethernet/intel/igc/igc_tsn.c | 25 +++++++-- > 4 files changed, 91 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 35baae900c1f..1067c46e0bc2 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -87,6 +87,7 @@ struct igc_ring { > u8 queue_index; /* logical index of the ring*/ > u8 reg_idx; /* physical index of the ring */ > bool launchtime_enable; /* true if LaunchTime is enabled */ > + bool preemptible; /* true if not express */ Mixing tabs and spaces? > +static int igc_ethtool_set_preempt(struct net_device *netdev, > + struct ethtool_fp *fpcmd, > + struct netlink_ext_ack *extack) > +{ > + struct igc_adapter *adapter = netdev_priv(netdev); > + int i; > + > + if (fpcmd->add_frag_size < 68 || fpcmd->add_frag_size > 260) { > + NL_SET_ERR_MSG_MOD(extack, "Invalid value for add-frag-size"); > + return -EINVAL; > + } This check should belong in ethtool, since there's nothing unusual about this supported range. Also, I believe that Jakub requested the min-frag-size to be passed as 0, 1, 2, 3 as the standard specifies it, and not its multiplied version? > + > + adapter->frame_preemption_active = fpcmd->enabled; > + adapter->add_frag_size = fpcmd->add_frag_size; > + > + if (!adapter->frame_preemption_active) > + goto done; > + > + /* Enabling frame preemption requires TSN mode to be enabled, > + * which requires a schedule to be active. So, if there isn't > + * a schedule already configured, configure a simple one, with > + * all queues open, with 1ms cycle time. > + */ > + if (adapter->base_time) > + goto done; Unless I'm missing something, you are interpreting an adapter->base_time value of zero as "no Qbv schedule on port", as if it was invalid to have a base-time of zero, which it isn't. > @@ -115,6 +130,9 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > if (ring->launchtime_enable) > txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT; > > + if (ring->preemptible) > + txqctl |= IGC_TXQCTL_PREEMPTABLE; I think this is the only place in the series where you use PREEMPTABLE instead of PREEMPTIBLE. > + > wr32(IGC_TXQCTL(i), txqctl); > } Out of curiosity, where is the ring to traffic class mapping configured in the igc driver? I suppose that you have more rings than traffic classes.
next prev parent reply other threads:[~2021-01-26 5:33 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-22 22:44 [PATCH net-next v3 0/8] ethtool: Add support for frame preemption Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 1/8] ethtool: Add support for configuring " Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-03-02 14:23 ` Vladimir Oltean 2021-03-02 14:23 ` [Intel-wired-lan] " Vladimir Oltean 2021-03-03 0:40 ` Vinicius Costa Gomes 2021-03-03 0:40 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-03-03 0:51 ` Vladimir Oltean 2021-03-03 0:51 ` [Intel-wired-lan] " Vladimir Oltean 2021-03-05 22:38 ` Vinicius Costa Gomes 2021-03-05 22:38 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 2/8] taprio: Add support for frame preemption offload Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-26 0:09 ` Vladimir Oltean 2021-01-26 0:09 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-29 21:13 ` Vinicius Costa Gomes 2021-01-29 21:13 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-29 21:57 ` Jakub Kicinski 2021-01-29 21:57 ` [Intel-wired-lan] " Jakub Kicinski 2021-01-29 23:12 ` Vinicius Costa Gomes 2021-01-29 23:12 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-30 0:27 ` Jakub Kicinski 2021-01-30 0:27 ` [Intel-wired-lan] " Jakub Kicinski 2021-01-29 23:20 ` Vladimir Oltean 2021-01-29 23:20 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-29 23:42 ` Vinicius Costa Gomes 2021-01-29 23:42 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-30 0:25 ` Vladimir Oltean 2021-01-30 0:25 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-22 22:44 ` [PATCH net-next v3 3/8] igc: Set the RX packet buffer size for TSN mode Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 4/8] igc: Only dump registers if configured to dump HW information Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 5/8] igc: Avoid TX Hangs because long cycles Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-26 0:02 ` Vladimir Oltean 2021-01-26 0:02 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-27 9:03 ` Kurt Kanzenbach 2021-01-27 9:03 ` [Intel-wired-lan] " Kurt Kanzenbach 2021-01-29 21:01 ` Vinicius Costa Gomes 2021-01-29 21:01 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 6/8] igc: Add support for tuning frame preemption via ethtool Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-26 0:32 ` Vladimir Oltean [this message] 2021-01-26 0:32 ` Vladimir Oltean 2021-01-29 21:27 ` Vinicius Costa Gomes 2021-01-29 21:27 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-29 23:16 ` Vladimir Oltean 2021-01-29 23:16 ` [Intel-wired-lan] " Vladimir Oltean 2021-03-03 1:07 ` Vladimir Oltean 2021-03-03 1:07 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-22 22:44 ` [PATCH net-next v3 7/8] igc: Add support for Frame Preemption offload Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-22 22:44 ` [PATCH net-next v3 8/8] igc: Separate TSN configurations that can be updated Vinicius Costa Gomes 2021-01-22 22:44 ` [Intel-wired-lan] " Vinicius Costa Gomes 2021-01-29 23:37 ` [PATCH net-next v3 0/8] ethtool: Add support for frame preemption Vladimir Oltean 2021-01-29 23:37 ` [Intel-wired-lan] " Vladimir Oltean 2021-01-30 0:11 ` Vinicius Costa Gomes 2021-01-30 0:11 ` [Intel-wired-lan] " Vinicius Costa Gomes
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210126003243.x3c44pmxmieqsa6e@skbuf \ --to=vladimir.oltean@nxp.com \ --cc=Jose.Abreu@synopsys.com \ --cc=anthony.l.nguyen@intel.com \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jhs@mojatatu.com \ --cc=jiri@resnulli.us \ --cc=kuba@kernel.org \ --cc=mkubecek@suse.cz \ --cc=netdev@vger.kernel.org \ --cc=po.liu@nxp.com \ --cc=vinicius.gomes@intel.com \ --cc=xiyou.wangcong@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.