| Message ID | 20250210070207.2615418-1-faizal.abdul.rahim@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | igc: Add support for Frame Preemption feature in IGC | expand |
On Mon, Feb 10, 2025 at 02:01:58AM -0500, Faizal Rahim wrote: > Introduces support for the FPE feature in the IGC driver. > > The patches aligns with the upstream FPE API: > https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/ > https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/ > > It builds upon earlier work: > https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/ > > The patch series adds the following functionalities to the IGC driver: > a) Configure FPE using `ethtool --set-mm`. > b) Display FPE settings via `ethtool --show-mm`. > c) View FPE statistics using `ethtool --include-statistics --show-mm'. > e) Enable preemptible/express queue with `fp`: > tc qdisc add ... root taprio \ > fp E E P P Any reason why you are only enabling the preemptible traffic classes with taprio, and not with mqprio as well? I see there will have to be some work harmonizing igc's existing understanding of ring priorities with what Kurt did in 9f3297511dae ("igc: Add MQPRIO offload support"), and I was kind of expecting to see a proposal for that as part of this.
On 13/2/2025 6:01 am, Vladimir Oltean wrote: > On Mon, Feb 10, 2025 at 02:01:58AM -0500, Faizal Rahim wrote: >> Introduces support for the FPE feature in the IGC driver. >> >> The patches aligns with the upstream FPE API: >> https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/ >> https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/ >> >> It builds upon earlier work: >> https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/ >> >> The patch series adds the following functionalities to the IGC driver: >> a) Configure FPE using `ethtool --set-mm`. >> b) Display FPE settings via `ethtool --show-mm`. >> c) View FPE statistics using `ethtool --include-statistics --show-mm'. >> e) Enable preemptible/express queue with `fp`: >> tc qdisc add ... root taprio \ >> fp E E P P > > Any reason why you are only enabling the preemptible traffic classes > with taprio, and not with mqprio as well? I see there will have to be > some work harmonizing igc's existing understanding of ring priorities > with what Kurt did in 9f3297511dae ("igc: Add MQPRIO offload support"), > and I was kind of expecting to see a proposal for that as part of this. > I was planning to enable fpe + mqprio separately since it requires extra effort to explore mqprio with preemptible rings, ring priorities, and testing to ensure it works properly and there are no regressions. I’m really hoping that fpe + mqprio doesn’t have to be enabled together in this series to keep things simple. It could be added later—adding it now would introduce additional complexity and delay this series further, which is focused on enabling basic, working fpe on i226. Would that be okay with you?
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Vladimir Oltean > Sent: Wednesday, February 12, 2025 11:01 PM > To: Faizal Rahim <faizal.abdul.rahim@linux.intel.com> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>; > David S . Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; > Alexandre Torgue <alexandre.torgue@foss.st.com>; Simon Horman > <horms@kernel.org>; Russell King <linux@armlinux.org.uk>; Alexei > Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; > Jesper Dangaard Brouer <hawk@kernel.org>; John Fastabend > <john.fastabend@gmail.com>; Furong Xu <0x1207@gmail.com>; Russell King > <rmk+kernel@armlinux.org.uk>; Serge Semin <fancer.lancer@gmail.com>; > Xiaolei Wang <xiaolei.wang@windriver.com>; Suraj Jaiswal > <quic_jsuraj@quicinc.com>; Kory Maincent <kory.maincent@bootlin.com>; > Gal Pressman <gal@nvidia.com>; Jesper Nilsson <jesper.nilsson@axis.com>; > Andrew Halaney <ahalaney@redhat.com>; Choong Yong Liang > <yong.liang.choong@linux.intel.com>; Kunihiko Hayashi > <hayashi.kunihiko@socionext.com>; Gomes, Vinicius > <vinicius.gomes@intel.com>; intel-wired-lan@lists.osuosl.org; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-stm32@st-md- > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; > bpf@vger.kernel.org > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v4 0/9] igc: Add support for > Frame Preemption feature in IGC Please start commit title from slam letters: Igc: add ... > On Mon, Feb 10, 2025 at 02:01:58AM -0500, Faizal Rahim wrote: > > Introduces support for the FPE feature in the IGC driver. > > > > The patches aligns with the upstream FPE API: > > > https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1 > 1 > > 56614-1-vladimir.oltean@nxp.com/ > > > https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.7 > 3 > > 054-1-vladimir.oltean@nxp.com/ > > > > It builds upon earlier work: > > > https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1 > 0 > > 98888-1-vinicius.gomes@intel.com/ > > > > The patch series adds the following functionalities to the IGC driver: > > a) Configure FPE using `ethtool --set-mm`. > > b) Display FPE settings via `ethtool --show-mm`. > > c) View FPE statistics using `ethtool --include-statistics --show-mm'. > > e) Enable preemptible/express queue with `fp`: > > tc qdisc add ... root taprio \ > > fp E E P P > > Any reason why you are only enabling the preemptible traffic classes with > taprio, and not with mqprio as well? I see there will have to be some work > harmonizing igc's existing understanding of ring priorities with what Kurt did in > 9f3297511dae ("igc: Add MQPRIO offload support"), and I was kind of > expecting to see a proposal for that as part of this.
On Thu, Feb 13, 2025 at 02:12:47PM +0800, Abdul Rahim, Faizal wrote: > I was planning to enable fpe + mqprio separately since it requires extra > effort to explore mqprio with preemptible rings, ring priorities, and > testing to ensure it works properly and there are no regressions. > > I’m really hoping that fpe + mqprio doesn’t have to be enabled together in > this series to keep things simple. It could be added later—adding it now > would introduce additional complexity and delay this series further, which > is focused on enabling basic, working fpe on i226. > > Would that be okay with you? But why would the mqprio params of taprio be handled differently than the dedicated mqprio qdisc? Why isn't the additional complexity you mention also needed for taprio? When I got convinced to expose preemptible TCs through separate UAPI in mqprio in taprio, it wasn't my understanding that drivers would be reacting differently depending on which Qdisc the configuration comes from.
On Thu, Feb 13, 2025 at 11:00:32AM +0200, Vladimir Oltean wrote: > On Thu, Feb 13, 2025 at 02:12:47PM +0800, Abdul Rahim, Faizal wrote: > > I was planning to enable fpe + mqprio separately since it requires extra > > effort to explore mqprio with preemptible rings, ring priorities, and > > testing to ensure it works properly and there are no regressions. > > > > I’m really hoping that fpe + mqprio doesn’t have to be enabled together in > > this series to keep things simple. It could be added later—adding it now > > would introduce additional complexity and delay this series further, which > > is focused on enabling basic, working fpe on i226. > > > > Would that be okay with you? > > But why would the mqprio params of taprio be handled differently than > the dedicated mqprio qdisc? Why isn't the additional complexity you > mention also needed for taprio? When I got convinced to expose > preemptible TCs through separate UAPI in mqprio in taprio, it wasn't my > understanding that drivers would be reacting differently depending on > which Qdisc the configuration comes from. If you want to reduce the complexity of individual patch sets, I guess you can keep this one for just the MAC Merge layer (ethtool), and then group common handling of preemptible traffic classes (both mqprio and taprio) in the next one.
On Thu Feb 13 2025, Abdul Rahim, Faizal wrote: > On 13/2/2025 6:01 am, Vladimir Oltean wrote: >> On Mon, Feb 10, 2025 at 02:01:58AM -0500, Faizal Rahim wrote: >>> Introduces support for the FPE feature in the IGC driver. >>> >>> The patches aligns with the upstream FPE API: >>> https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/ >>> https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/ >>> >>> It builds upon earlier work: >>> https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/ >>> >>> The patch series adds the following functionalities to the IGC driver: >>> a) Configure FPE using `ethtool --set-mm`. >>> b) Display FPE settings via `ethtool --show-mm`. >>> c) View FPE statistics using `ethtool --include-statistics --show-mm'. >>> e) Enable preemptible/express queue with `fp`: >>> tc qdisc add ... root taprio \ >>> fp E E P P >> >> Any reason why you are only enabling the preemptible traffic classes >> with taprio, and not with mqprio as well? I see there will have to be >> some work harmonizing igc's existing understanding of ring priorities >> with what Kurt did in 9f3297511dae ("igc: Add MQPRIO offload support"), >> and I was kind of expecting to see a proposal for that as part of this. >> > > I was planning to enable fpe + mqprio separately since it requires extra > effort to explore mqprio with preemptible rings, ring priorities, and > testing to ensure it works properly and there are no regressions. Well, my idea was to move the current mqprio offload implementation from legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio can share the same code (with or without fpe). I have a draft patch ready for that. What do you think about it? Thanks, Kurt
On 13/2/2025 8:01 pm, Kurt Kanzenbach wrote: > On Thu Feb 13 2025, Abdul Rahim, Faizal wrote: >> On 13/2/2025 6:01 am, Vladimir Oltean wrote: >>> On Mon, Feb 10, 2025 at 02:01:58AM -0500, Faizal Rahim wrote: >>>> Introduces support for the FPE feature in the IGC driver. >>>> >>>> The patches aligns with the upstream FPE API: >>>> https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/ >>>> https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/ >>>> >>>> It builds upon earlier work: >>>> https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/ >>>> >>>> The patch series adds the following functionalities to the IGC driver: >>>> a) Configure FPE using `ethtool --set-mm`. >>>> b) Display FPE settings via `ethtool --show-mm`. >>>> c) View FPE statistics using `ethtool --include-statistics --show-mm'. >>>> e) Enable preemptible/express queue with `fp`: >>>> tc qdisc add ... root taprio \ >>>> fp E E P P >>> >>> Any reason why you are only enabling the preemptible traffic classes >>> with taprio, and not with mqprio as well? I see there will have to be >>> some work harmonizing igc's existing understanding of ring priorities >>> with what Kurt did in 9f3297511dae ("igc: Add MQPRIO offload support"), >>> and I was kind of expecting to see a proposal for that as part of this. >>> >> >> I was planning to enable fpe + mqprio separately since it requires extra >> effort to explore mqprio with preemptible rings, ring priorities, and >> testing to ensure it works properly and there are no regressions. > > Well, my idea was to move the current mqprio offload implementation from > legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio > can share the same code (with or without fpe). I have a draft patch > ready for that. What do you think about it? > > Thanks, > Kurt Hi Kurt, I’m okay with including it in this series and testing fpe + mqprio, but I’m not sure if others might be concerned about adding different functional changes in this fpe series. Hi Vladimir, Any thoughts on this ?
On Thu, Feb 13, 2025 at 08:54:18PM +0800, Abdul Rahim, Faizal wrote: > > Well, my idea was to move the current mqprio offload implementation from > > legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio > > can share the same code (with or without fpe). I have a draft patch > > ready for that. What do you think about it? > > Hi Kurt, > > I’m okay with including it in this series and testing fpe + mqprio, but I’m > not sure if others might be concerned about adding different functional > changes in this fpe series. > > Hi Vladimir, > Any thoughts on this ? Well, what do you think of my split proposal from here, essentially drawing the line for the first patch set at just ethtool mm? https://lore.kernel.org/netdev/20250213110653.iqy5magn27jyfnwh@skbuf/
On 13/2/2025 9:00 pm, Vladimir Oltean wrote: > On Thu, Feb 13, 2025 at 08:54:18PM +0800, Abdul Rahim, Faizal wrote: >>> Well, my idea was to move the current mqprio offload implementation from >>> legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio >>> can share the same code (with or without fpe). I have a draft patch >>> ready for that. What do you think about it? >> >> Hi Kurt, >> >> I’m okay with including it in this series and testing fpe + mqprio, but I’m >> not sure if others might be concerned about adding different functional >> changes in this fpe series. >> >> Hi Vladimir, >> Any thoughts on this ? > > Well, what do you think of my split proposal from here, essentially > drawing the line for the first patch set at just ethtool mm? > https://lore.kernel.org/netdev/20250213110653.iqy5magn27jyfnwh@skbuf/ > Honestly, after reconsidering, I’d prefer to keep the current series as is (without Kurt’s patch), assuming you’re okay with enabling mqprio + fpe later rather than at the same time as taprio + fpe. There likely won’t be any additional work needed for mqprio + fpe after Kurt’s patch is accepted, since it will mostly reuse the taprio code flow. If I were to split it, the structure would look something like this: First part of fpe series: igc: Add support to get frame preemption statistics via ethtool igc: Add support to get MAC Merge data via ethtool igc: Add support to set tx-min-frag-size igc: Add support for frame preemption verification igc: Set the RX packet buffer size for TSN mode igc: Optimize the TX packet buffer utilization igc: Rename xdp_get_tx_ring() for non-XDP usage net: ethtool: mm: Extract stmmac verification logic into a common library Second part of fpe: igc: Add support for preemptible traffic class in taprio I don’t think Kurt’s patch should be included in my second part of fpe, as it’s not logically related. Another approach would be to wait for Kurt’s patch to be accepted first, then submit the second part and verify both taprio + mqprio. However, that would delay i226 from having a basic fpe feature working as a whole, which I'd really like to avoid.
On Thu Feb 13 2025, Abdul Rahim, Faizal wrote: > On 13/2/2025 9:00 pm, Vladimir Oltean wrote: >> On Thu, Feb 13, 2025 at 08:54:18PM +0800, Abdul Rahim, Faizal wrote: >>>> Well, my idea was to move the current mqprio offload implementation from >>>> legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio >>>> can share the same code (with or without fpe). I have a draft patch >>>> ready for that. What do you think about it? >>> >>> Hi Kurt, >>> >>> I’m okay with including it in this series and testing fpe + mqprio, but I’m >>> not sure if others might be concerned about adding different functional >>> changes in this fpe series. >>> >>> Hi Vladimir, >>> Any thoughts on this ? >> >> Well, what do you think of my split proposal from here, essentially >> drawing the line for the first patch set at just ethtool mm? >> https://lore.kernel.org/netdev/20250213110653.iqy5magn27jyfnwh@skbuf/ >> > > Honestly, after reconsidering, I’d prefer to keep the current series as is > (without Kurt’s patch), assuming you’re okay with enabling mqprio + fpe > later rather than at the same time as taprio + fpe. There likely won’t be > any additional work needed for mqprio + fpe after Kurt’s patch is accepted, > since it will mostly reuse the taprio code flow. I think so. After switching the Tx mode mqprio will basically be a special case of taprio with a dummy Qbv schedule. Also the driver currently rejects mqprio with hardware offloading and preemptible_tcs set. So, I do not see any issues in merging your fpe series first. I can handle the mqprio part afterwards. Thanks, Kurt
On Thu, Feb 13, 2025 at 03:12:35PM +0100, Kurt Kanzenbach wrote: > On Thu Feb 13 2025, Abdul Rahim, Faizal wrote: > > On 13/2/2025 9:00 pm, Vladimir Oltean wrote: > >> On Thu, Feb 13, 2025 at 08:54:18PM +0800, Abdul Rahim, Faizal wrote: > >>>> Well, my idea was to move the current mqprio offload implementation from > >>>> legacy TSN Tx mode to the normal TSN Tx mode. Then, taprio and mqprio > >>>> can share the same code (with or without fpe). I have a draft patch > >>>> ready for that. What do you think about it? > >>> > >>> Hi Kurt, > >>> > >>> I’m okay with including it in this series and testing fpe + mqprio, but I’m > >>> not sure if others might be concerned about adding different functional > >>> changes in this fpe series. > >>> > >>> Hi Vladimir, > >>> Any thoughts on this ? > >> > >> Well, what do you think of my split proposal from here, essentially > >> drawing the line for the first patch set at just ethtool mm? > >> https://lore.kernel.org/netdev/20250213110653.iqy5magn27jyfnwh@skbuf/ > >> > > > > Honestly, after reconsidering, I’d prefer to keep the current series as is > > (without Kurt’s patch), assuming you’re okay with enabling mqprio + fpe > > later rather than at the same time as taprio + fpe. There likely won’t be > > any additional work needed for mqprio + fpe after Kurt’s patch is accepted, > > since it will mostly reuse the taprio code flow. > > I think so. After switching the Tx mode mqprio will basically be a > special case of taprio with a dummy Qbv schedule. Also the driver > currently rejects mqprio with hardware offloading and preemptible_tcs > set. So, I do not see any issues in merging your fpe series first. I can > handle the mqprio part afterwards. > > Thanks, > Kurt Currently, igc sets tc_taprio_caps :: broken_mqprio = true, meaning that higher scheduling priority is given to smaller TXQ indices. This is a special case, as normally speaking, higher scheduler priority is given to higher traffic classes, both in mqprio and in normal taprio (see taprio_dequeue_txq_priority() vs taprio_dequeue_tc_priority()). In commit 9f3297511dae ("igc: Add MQPRIO offload support") you document the intended mqprio usage pattern: tc qdisc replace dev ${INTERFACE} handle 100 parent root mqprio num_tc 4 \ map 0 0 0 0 0 1 2 3 0 0 0 0 0 0 0 0 \ queues 1@0 1@1 1@2 1@3 \ hw 1 Applying the transformations described in https://man7.org/linux/man-pages/man8/tc-mqprio.8.html, it looks like this: ┌────┬────┬───────┐ │Prio│ tc │ queue │ ├────┼────┼───────┤ │ 0 │ 0 │ 0 │ │ 1 │ 0 │ 0 │ │ 2 │ 0 │ 0 │ │ 3 │ 0 │ 0 │ │ 4 │ 0 │ 0 │ │ 5 │ 1 │ 1 │ │ 6 │ 2 │ 2 │ │ 7 │ 3 │ 3 │ │ 8 │ 0 │ 0 │ │ 9 │ 0 │ 0 │ │ 10 │ 0 │ 0 │ │ 11 │ 0 │ 0 │ │ 12 │ 0 │ 0 │ │ 13 │ 0 │ 0 │ │ 14 │ 0 │ 0 │ │ 15 │ 0 │ 0 │ └────┴────┴───────┘ In this model, prio 7 goes to TXQ 3, and since I assume prio 7 is a high priority, it makes me think TXQ 3 is the highest priority queue (I don't have a lot of spare time to search for i216 documentation and enlighten myself). Then we have Faizal's example from patch 7/9: https://lore.kernel.org/netdev/20250210070207.2615418-8-faizal.abdul.rahim@linux.intel.com/ a) 1:1 TC-to-Queue Mapping $ sudo tc qdisc replace dev enp1s0 parent root handle 100 \ taprio num_tc 4 map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \ queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \ fp E E P P b) Non-1:1 TC-to-Queue Mapping $ sudo tc qdisc replace dev enp1s0 parent root handle 100 \ taprio num_tc 3 map 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 2 queues 2@0 1@2 1@3 fp E E P ┌────┬────┬───────┐ ┌────┬────┬────────┐ │Prio│ tc │ queue │ │Prio│ tc │ queue │ ├────┼────┼───────┤ ├────┼────┼────────┤ │ 0 │ 3 │ 3 │ │ 0 │ 2 │ 3 │ │ 1 │ 2 │ 2 │ │ 1 │ 1 │ 2 │ │ 2 │ 1 │ 1 │ │ 2 │ 0 │ 0 or 1 │ │ 3 │ 0 │ 0 │ │ 3 │ 2 │ 3 │ │ 4 │ 3 │ 3 │ │ 4 │ 2 │ 3 │ │ 5 │ 3 │ 3 │ │ 5 │ 2 │ 3 │ │ 6 │ 3 │ 3 │ │ 6 │ 2 │ 3 │ │ 7 │ 3 │ 3 │ │ 7 │ 2 │ 3 │ │ 8 │ 3 │ 3 │ │ 8 │ 2 │ 3 │ │ 9 │ 3 │ 3 │ │ 9 │ 2 │ 3 │ │ 10 │ 3 │ 3 │ │ 10 │ 2 │ 3 │ │ 11 │ 3 │ 3 │ │ 11 │ 2 │ 3 │ │ 12 │ 3 │ 3 │ │ 12 │ 2 │ 3 │ │ 13 │ 3 │ 3 │ │ 13 │ 2 │ 3 │ │ 14 │ 3 │ 3 │ │ 14 │ 2 │ 3 │ │ 15 │ 3 │ 3 │ │ 15 │ 2 │ 3 │ └────┴────┴───────┘ └────┴────┴────────┘ case a case b In these cases, Faizal leaves us a hint that the preemptible traffic classes are the ones with the lower scheduling priority (TC2 and TC3 in case a, TC2 in case b). Here, the lower scheduling priority traffic classes are mapped to the higher numbered TX queues, which basically matches the broken_mqprio description. So, confusingly to me, it seems like one operating mode is fundamentally different from the other, and something will have to change if both will be made to behave the same. What will change? You say mqprio will behave like taprio, but I think if anything, mqprio is the one which does the right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx arbitration scheme? I don't think I'm on the same page as you guys, because to me, it is just odd that the P traffic classes would be the first ones with mqprio, but the last ones with taprio.
On Thu Feb 13 2025, Vladimir Oltean wrote: > So, confusingly to me, it seems like one operating mode is fundamentally > different from the other, and something will have to change if both will > be made to behave the same. What will change? You say mqprio will behave > like taprio, but I think if anything, mqprio is the one which does the > right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx > arbitration scheme? Correct. taprio is using the default scheme. mqprio configures it to what ever the user provided (in igc_tsn_tx_arb()). > I don't think I'm on the same page as you guys, because to me, it is > just odd that the P traffic classes would be the first ones with > mqprio, but the last ones with taprio. I think we are on the same page here. At the end both have to behave the same. Either by using igc_tsn_tx_arb() for taprio too or only using the default scheme for both (and thereby keeping broken_mqprio). Whatever Faizal implements I'll match the behavior with mqprio. Thanks, Kurt
On 14/2/2025 3:12 am, Kurt Kanzenbach wrote: > On Thu Feb 13 2025, Vladimir Oltean wrote: >> So, confusingly to me, it seems like one operating mode is fundamentally >> different from the other, and something will have to change if both will >> be made to behave the same. What will change? You say mqprio will behave >> like taprio, but I think if anything, mqprio is the one which does the >> right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx >> arbitration scheme? > > Correct. taprio is using the default scheme. mqprio configures it to > what ever the user provided (in igc_tsn_tx_arb()). > >> I don't think I'm on the same page as you guys, because to me, it is >> just odd that the P traffic classes would be the first ones with >> mqprio, but the last ones with taprio. > > I think we are on the same page here. At the end both have to behave the > same. Either by using igc_tsn_tx_arb() for taprio too or only using the > default scheme for both (and thereby keeping broken_mqprio). Whatever > Faizal implements I'll match the behavior with mqprio. > Hi Kurt & Vladimir, After reading Vladimir's reply on tc, hw queue, and socket priority mapping for both taprio and mqprio, I agree they should follow the same priority scheme for consistency—both in code and command usage (i.e., taprio, mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a standard mapping of tc, socket priority, and hardware queue priority, I'll enable taprio to use igc_tsn_tx_arb() in a separate patch submission. I'll split the changes based on Vladimir's suggestion. First part - ethtool-mm related: igc: Add support to get frame preemption statistics via ethtool igc: Add support to get MAC Merge data via ethtool igc: Add support to set tx-min-frag-size igc: Add support for frame preemption verification igc: Set the RX packet buffer size for TSN mode igc: Optimize TX packet buffer utilization igc: Rename xdp_get_tx_ring() for non-XDP usage net: ethtool: mm: Extract stmmac verification logic into a common library Second part: igc: Add support for preemptible traffic class in taprio and mqprio igc: Use igc_tsn_tx_arb() for taprio queue priority scheme igc: Kurt's patch on mqprio to use normal TSN Tx mode Kurt can keep igc_tsn_tx_arb() for his mqprio patch, so preemptible tc should work the same for both taprio and mqprio. I'm suggesting to include Kurt's patch in the second part since there's some dependency and potential code conflict, even though it mixes different functional changes in the same series. Does this sound good to you?
On 14/2/2025 12:20 pm, Abdul Rahim, Faizal wrote: > > > On 14/2/2025 3:12 am, Kurt Kanzenbach wrote: >> On Thu Feb 13 2025, Vladimir Oltean wrote: >>> So, confusingly to me, it seems like one operating mode is fundamentally >>> different from the other, and something will have to change if both will >>> be made to behave the same. What will change? You say mqprio will behave >>> like taprio, but I think if anything, mqprio is the one which does the >>> right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx >>> arbitration scheme? >> >> Correct. taprio is using the default scheme. mqprio configures it to >> what ever the user provided (in igc_tsn_tx_arb()). >> >>> I don't think I'm on the same page as you guys, because to me, it is >>> just odd that the P traffic classes would be the first ones with >>> mqprio, but the last ones with taprio. >> >> I think we are on the same page here. At the end both have to behave the >> same. Either by using igc_tsn_tx_arb() for taprio too or only using the >> default scheme for both (and thereby keeping broken_mqprio). Whatever >> Faizal implements I'll match the behavior with mqprio. >> > > Hi Kurt & Vladimir, > > After reading Vladimir's reply on tc, hw queue, and socket priority mapping > for both taprio and mqprio, I agree they should follow the same priority > scheme for consistency—both in code and command usage (i.e., taprio, > mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a > standard mapping of tc, socket priority, and hardware queue priority, I'll > enable taprio to use igc_tsn_tx_arb() in a separate patch submission. > > I'll split the changes based on Vladimir's suggestion. > > First part - ethtool-mm related: > igc: Add support to get frame preemption statistics via ethtool > igc: Add support to get MAC Merge data via ethtool > igc: Add support to set tx-min-frag-size > igc: Add support for frame preemption verification > igc: Set the RX packet buffer size for TSN mode > igc: Optimize TX packet buffer utilization > igc: Rename xdp_get_tx_ring() for non-XDP usage > net: ethtool: mm: Extract stmmac verification logic into a common library > > Second part: > igc: Add support for preemptible traffic class in taprio and mqprio > igc: Use igc_tsn_tx_arb() for taprio queue priority scheme > igc: Kurt's patch on mqprio to use normal TSN Tx mode > > Kurt can keep igc_tsn_tx_arb() for his mqprio patch, so preemptible tc > should work the same for both taprio and mqprio. > > I'm suggesting to include Kurt's patch in the second part since there's > some dependency and potential code conflict, even though it mixes different > functional changes in the same series. I forgot that the second part patch: igc: Add support for preemptible traffic class in taprio and mqprio depends on the first part ethtool-mm, which would delay Kurt's patch. So Kurt, if you'd prefer to submit yours first, that's okay too.
On Fri Feb 14 2025, Abdul Rahim, Faizal wrote: > On 14/2/2025 3:12 am, Kurt Kanzenbach wrote: >> On Thu Feb 13 2025, Vladimir Oltean wrote: >>> So, confusingly to me, it seems like one operating mode is fundamentally >>> different from the other, and something will have to change if both will >>> be made to behave the same. What will change? You say mqprio will behave >>> like taprio, but I think if anything, mqprio is the one which does the >>> right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx >>> arbitration scheme? >> >> Correct. taprio is using the default scheme. mqprio configures it to >> what ever the user provided (in igc_tsn_tx_arb()). >> >>> I don't think I'm on the same page as you guys, because to me, it is >>> just odd that the P traffic classes would be the first ones with >>> mqprio, but the last ones with taprio. >> >> I think we are on the same page here. At the end both have to behave the >> same. Either by using igc_tsn_tx_arb() for taprio too or only using the >> default scheme for both (and thereby keeping broken_mqprio). Whatever >> Faizal implements I'll match the behavior with mqprio. >> > > Hi Kurt & Vladimir, > > After reading Vladimir's reply on tc, hw queue, and socket priority mapping > for both taprio and mqprio, I agree they should follow the same priority > scheme for consistency—both in code and command usage (i.e., taprio, > mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a > standard mapping of tc, socket priority, and hardware queue priority, I'll > enable taprio to use igc_tsn_tx_arb() in a separate patch submission. There's one point to consider here: igc_tsn_tx_arb() changes the mapping between priorities and Tx queues. I have no idea how many people rely on the fact that queue 0 has always the highest priority. For example, it will change the Tx behavior for schedules which open multiple traffic classes at the same time. Users may notice. OTOH changing mqprio to the broken_mqprio model is easy, because AFAIK there's only one customer using this. Thanks, Kurt
On 14/2/2025 4:56 pm, Kurt Kanzenbach wrote: > On Fri Feb 14 2025, Abdul Rahim, Faizal wrote: >> On 14/2/2025 3:12 am, Kurt Kanzenbach wrote: >>> On Thu Feb 13 2025, Vladimir Oltean wrote: >>>> So, confusingly to me, it seems like one operating mode is fundamentally >>>> different from the other, and something will have to change if both will >>>> be made to behave the same. What will change? You say mqprio will behave >>>> like taprio, but I think if anything, mqprio is the one which does the >>>> right thing, in igc_tsn_tx_arb(), and taprio seems to use the default Tx >>>> arbitration scheme? >>> >>> Correct. taprio is using the default scheme. mqprio configures it to >>> what ever the user provided (in igc_tsn_tx_arb()). >>> >>>> I don't think I'm on the same page as you guys, because to me, it is >>>> just odd that the P traffic classes would be the first ones with >>>> mqprio, but the last ones with taprio. >>> >>> I think we are on the same page here. At the end both have to behave the >>> same. Either by using igc_tsn_tx_arb() for taprio too or only using the >>> default scheme for both (and thereby keeping broken_mqprio). Whatever >>> Faizal implements I'll match the behavior with mqprio. >>> >> >> Hi Kurt & Vladimir, >> >> After reading Vladimir's reply on tc, hw queue, and socket priority mapping >> for both taprio and mqprio, I agree they should follow the same priority >> scheme for consistency—both in code and command usage (i.e., taprio, >> mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a >> standard mapping of tc, socket priority, and hardware queue priority, I'll >> enable taprio to use igc_tsn_tx_arb() in a separate patch submission. > > There's one point to consider here: igc_tsn_tx_arb() changes the mapping > between priorities and Tx queues. I have no idea how many people rely on > the fact that queue 0 has always the highest priority. For example, it > will change the Tx behavior for schedules which open multiple traffic > classes at the same time. Users may notice. Yeah, I was considering the impact on existing users too. I hadn’t given it much thought initially and figured they’d just need to adapt to the changes, but now that I think about it, properly communicating this would be tough. taprio on igc (i225, i226) has been around for a while, so a lot of users would be affected. > OTOH changing mqprio to the broken_mqprio model is easy, because AFAIK > there's only one customer using this. > Hmmmm, now I’m leaning toward keeping taprio as is (hw queue 0 highest priority) and having mqprio follow the default priority scheme (aka broken_mqprio). Even though it’s not the norm, the impact doesn’t seem worth the gain. Open to hearing others' thoughts.
Faizal, On Fri, Feb 14, 2025 at 05:43:19PM +0800, Abdul Rahim, Faizal wrote: > > > Hi Kurt & Vladimir, > > > > > > After reading Vladimir's reply on tc, hw queue, and socket priority mapping > > > for both taprio and mqprio, I agree they should follow the same priority > > > scheme for consistency—both in code and command usage (i.e., taprio, > > > mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a > > > standard mapping of tc, socket priority, and hardware queue priority, I'll > > > enable taprio to use igc_tsn_tx_arb() in a separate patch submission. > > > > There's one point to consider here: igc_tsn_tx_arb() changes the mapping > > between priorities and Tx queues. I have no idea how many people rely on > > the fact that queue 0 has always the highest priority. For example, it > > will change the Tx behavior for schedules which open multiple traffic > > classes at the same time. Users may notice. > > Yeah, I was considering the impact on existing users too. I hadn’t given it > much thought initially and figured they’d just need to adapt to the changes, > but now that I think about it, properly communicating this would be tough. > taprio on igc (i225, i226) has been around for a while, so a lot of users > would be affected. > > > OTOH changing mqprio to the broken_mqprio model is easy, because AFAIK > > there's only one customer using this. > > > > Hmmmm, now I’m leaning toward keeping taprio as is (hw queue 0 highest > priority) and having mqprio follow the default priority scheme (aka > broken_mqprio). Even though it’s not the norm, the impact doesn’t seem worth > the gain. Open to hearing others' thoughts. Kurt is right, you need to think about your users, but it isn't only that. Intel puts out a lot of user-facing TSN technical documentation for Linux, and currently, they have a hard time adapting it to other vendors, because of Intel specific peculiarities such as this one. I would argue that for being one of the most visible vendors from the Linux TSN space, you also have a duty to the rest of the community of not pushing users away from established conventions. It's unfair that a past design mistake would stifle further evolution of the driver in the correct direction, so I don't think we should let that happen. I was thinking the igc driver should have a driver-specific opt-in flag which users explicitly have to set in order to get the conventional TX scheduling behavior in taprio (the one from mqprio). Public Intel documentation would be updated to present the differences between the old and the new mode, and to recommend opting into the new mode. By default, the current behavior is maintained, thus not breaking any user. Something like an ethtool priv flag seems adequate for this. Understandably, many network maintainers will initially dislike this, but you will have to be persistent and explain the ways in which having this priv flag is better than not having it. Normally they will respect those reasons more than they dislike driver-specific priv flags, which, let's be honest, are way too often abused for adding custom behavior. Here the situation is different, the custom behavior already exists, it just doesn't have a name and there's no way of turning it off.
On 14/2/2025 6:22 pm, Vladimir Oltean wrote: > Faizal, > > On Fri, Feb 14, 2025 at 05:43:19PM +0800, Abdul Rahim, Faizal wrote: >>>> Hi Kurt & Vladimir, >>>> >>>> After reading Vladimir's reply on tc, hw queue, and socket priority mapping >>>> for both taprio and mqprio, I agree they should follow the same priority >>>> scheme for consistency—both in code and command usage (i.e., taprio, >>>> mqprio, and fpe in both configurations). Since igc_tsn_tx_arb() ensures a >>>> standard mapping of tc, socket priority, and hardware queue priority, I'll >>>> enable taprio to use igc_tsn_tx_arb() in a separate patch submission. >>> >>> There's one point to consider here: igc_tsn_tx_arb() changes the mapping >>> between priorities and Tx queues. I have no idea how many people rely on >>> the fact that queue 0 has always the highest priority. For example, it >>> will change the Tx behavior for schedules which open multiple traffic >>> classes at the same time. Users may notice. >> >> Yeah, I was considering the impact on existing users too. I hadn’t given it >> much thought initially and figured they’d just need to adapt to the changes, >> but now that I think about it, properly communicating this would be tough. >> taprio on igc (i225, i226) has been around for a while, so a lot of users >> would be affected. >> >>> OTOH changing mqprio to the broken_mqprio model is easy, because AFAIK >>> there's only one customer using this. >>> >> >> Hmmmm, now I’m leaning toward keeping taprio as is (hw queue 0 highest >> priority) and having mqprio follow the default priority scheme (aka >> broken_mqprio). Even though it’s not the norm, the impact doesn’t seem worth >> the gain. Open to hearing others' thoughts. > > Kurt is right, you need to think about your users, but it isn't only that. > Intel puts out a lot of user-facing TSN technical documentation for Linux, > and currently, they have a hard time adapting it to other vendors, because > of Intel specific peculiarities such as this one. I would argue that for > being one of the most visible vendors from the Linux TSN space, you also > have a duty to the rest of the community of not pushing users away from > established conventions. > > It's unfair that a past design mistake would stifle further evolution of > the driver in the correct direction, so I don't think we should let that > happen. I was thinking the igc driver should have a driver-specific > opt-in flag which users explicitly have to set in order to get the > conventional TX scheduling behavior in taprio (the one from mqprio). > Public Intel documentation would be updated to present the differences > between the old and the new mode, and to recommend opting into the new > mode. By default, the current behavior is maintained, thus not breaking > any user. Something like an ethtool priv flag seems adequate for this. > > Understandably, many network maintainers will initially dislike this, > but you will have to be persistent and explain the ways in which having > this priv flag is better than not having it. Normally they will respect > those reasons more than they dislike driver-specific priv flags, which, > let's be honest, are way too often abused for adding custom behavior. > Here the situation is different, the custom behavior already exists, it > just doesn't have a name and there's no way of turning it off. Okay. I can look into this in a separate patch submission, but just an FYI—this adds another dependency to the second part of the igc fpe submission (preemptible tc on taprio + mqprio). This new patch (driver-specific priv flag to control 2 different priority scheme) would need to be accepted first before the second part of igc fpe can be submitted.
On Fri, Feb 14, 2025 at 07:20:08PM +0800, Abdul Rahim, Faizal wrote: > Okay. I can look into this in a separate patch submission, but just an > FYI—this adds another dependency to the second part of the igc fpe > submission (preemptible tc on taprio + mqprio). This new patch > (driver-specific priv flag to control 2 different priority scheme) would > need to be accepted first before the second part of igc fpe can be > submitted. So perhaps now you're starting to understand why I had initially suggested you'd better draw the line now at just the MAC Merge layer and focus on harmonizing taprio and mqprio TX scheduling in a separate patch set. I would expect that for uniform behavior, you would force the users a little bit to adopt the new TX scheduling mode in taprio, otherwise any configuration with preemptible traffic classes would be rejected by the driver. So, if preemption is used, then the scheduling model is the same between mqprio and taprio, and you don't have to handle preemptible traffic classes over the old scheduling model. I would also expect that you replace the current patch which handles preemptible traffic classes in taprio with another one which explicitly returns -EOPNOTSUPP if preemptible traffic classes exist - just like mqprio. When Kurt added that condition in mqprio, it wasn't strictly necessary, because mqprio_parse_tc_entries() already checks ethtool_dev_mm_supported() in the core and rejects the configuration. But after your MAC Merge series is accepted, you still won't be able to handle preemptible traffic classes even though the core will let you, so you will have to impose the restriction yourself, just like Kurt did. I'm not trying to be negative, but it's imaginable that there's a chance you won't succeed to bring the whole feature set to completion right away, and if you do abandon things in the middle (MAC Merge layer supported but preemptible traffic classes unsupported), it would be good if the driver at least rejected a configuration it doesn't support, instead of accepting it and not acting on it. Because if a significant amount of time passes in such an inconsistent state, it would be very difficult for anybody else to pick up from that position and not change the behavior in incompatible ways that are user-visible.
On 13/2/2025 4:59 pm, Loktionov, Aleksandr wrote: > > >> -----Original Message----- >> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >> Vladimir Oltean >> Sent: Wednesday, February 12, 2025 11:01 PM >> To: Faizal Rahim <faizal.abdul.rahim@linux.intel.com> >> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw >> <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>; >> David S . Miller <davem@davemloft.net>; Eric Dumazet >> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni >> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; >> Alexandre Torgue <alexandre.torgue@foss.st.com>; Simon Horman >> <horms@kernel.org>; Russell King <linux@armlinux.org.uk>; Alexei >> Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; >> Jesper Dangaard Brouer <hawk@kernel.org>; John Fastabend >> <john.fastabend@gmail.com>; Furong Xu <0x1207@gmail.com>; Russell King >> <rmk+kernel@armlinux.org.uk>; Serge Semin <fancer.lancer@gmail.com>; >> Xiaolei Wang <xiaolei.wang@windriver.com>; Suraj Jaiswal >> <quic_jsuraj@quicinc.com>; Kory Maincent <kory.maincent@bootlin.com>; >> Gal Pressman <gal@nvidia.com>; Jesper Nilsson <jesper.nilsson@axis.com>; >> Andrew Halaney <ahalaney@redhat.com>; Choong Yong Liang >> <yong.liang.choong@linux.intel.com>; Kunihiko Hayashi >> <hayashi.kunihiko@socionext.com>; Gomes, Vinicius >> <vinicius.gomes@intel.com>; intel-wired-lan@lists.osuosl.org; >> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-stm32@st-md- >> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; >> bpf@vger.kernel.org >> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v4 0/9] igc: Add support for >> Frame Preemption feature in IGC > > Please start commit title from slam letters: > Igc: add ... Hi Aleksandr, I haven't updated this in v5 yet. Could you share any reference or guideline for this? From what I checked, the recently accepted patches in igc seem to follow a similar commit title format as my patches: $ git log --oneline | grep igc b65969856d4f igc: Link queues to NAPI instances 1a63399c13fe igc: Link IRQs to NAPI instances 8b6237e1f4d4 igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog() 484d3675f2aa igc: Allow hot-swapping XDP program c75889081366 igc: Remove unused igc_read/write_pcie_cap_reg 121c3c6bc661 igc: Remove unused igc_read/write_pci_cfg wrappers b37dba891b17 igc: Remove unused igc_acquire/release_nvm
Introduces support for the FPE feature in the IGC driver. The patches aligns with the upstream FPE API: https://patchwork.kernel.org/project/netdevbpf/cover/20230220122343.1156614-1-vladimir.oltean@nxp.com/ https://patchwork.kernel.org/project/netdevbpf/cover/20230119122705.73054-1-vladimir.oltean@nxp.com/ It builds upon earlier work: https://patchwork.kernel.org/project/netdevbpf/cover/20220520011538.1098888-1-vinicius.gomes@intel.com/ The patch series adds the following functionalities to the IGC driver: a) Configure FPE using `ethtool --set-mm`. b) Display FPE settings via `ethtool --show-mm`. c) View FPE statistics using `ethtool --include-statistics --show-mm'. e) Enable preemptible/express queue with `fp`: tc qdisc add ... root taprio \ fp E E P P Change Log: v3 -> v4: - Fix compilation warnings introduced by this patch series v2 -> v3: - Implement configure_tx() mmsv callback (Vladimir) - Use static_branch_inc() and static_branch_dec() (Vladimir) - Add adapter->fpe.mmsv.pmac_enabled as extra check (Vladimir) - Remove unnecessary error check in igc_fpe_init_tx_descriptor() (Vladimir) - Additional places to use FIELD_PREP() instead of manual bit manipulation (Vladimir) - IGC_TXD_POPTS_SMD_V and IGC_TXD_POPTS_SMD_R type change to enum (Vladimir) - Remove unnecessary netif_running() check in igc_fpe_xmit_frame (Vladimir) - Rate limit print in igc_fpe_send_mpacket (Vladimir) v1 -> v2: - Extract the stmmac verification logic into a common library (Vladimir) - igc to use common library for verification (Vladimir) - Fix syntax for kernel-doc to use "Return:" (Vladimir) - Use FIELD_GET instead of manual bit masking (Vladimir) - Don't assign 0 to statistics counter in igc_ethtool_get_mm_stats() (Vladimir) - Use pmac-enabled as a condition to allow MAC address value 0 (Vladimir) - Define macro register value in increasing value order (Vladimir) - Fix tx-min-frag-size handling for igc (Vladimir) - Handle link state changes with verification in igc (Vladimir) - Add static key for fast path code (Vladimir) - rx_min_frag_size get from constant (Vladimir) v1: https://patchwork.kernel.org/project/netdevbpf/cover/20241216064720.931522-1-faizal.abdul.rahim@linux.intel.com/ v2: https://patchwork.kernel.org/project/netdevbpf/cover/20250205100524.1138523-1-faizal.abdul.rahim@linux.intel.com/ v3: https://patchwork.kernel.org/project/netdevbpf/cover/20250207165649.2245320-1-faizal.abdul.rahim@linux.intel.com/ Faizal Rahim (8): igc: Rename xdp_get_tx_ring() for non-xdp usage igc: Optimize the TX packet buffer utilization igc: Set the RX packet buffer size for TSN mode igc: Add support for frame preemption verification igc: Add support to set tx-min-frag-size igc: Add support for preemptible traffic class in taprio igc: Add support to get MAC Merge data via ethtool igc: Add support to get frame preemption statistics via ethtool Vladimir Oltean (1): net: ethtool: mm: extract stmmac verification logic into common library drivers/net/ethernet/intel/igc/igc.h | 18 +- drivers/net/ethernet/intel/igc/igc_base.h | 1 + drivers/net/ethernet/intel/igc/igc_defines.h | 16 +- drivers/net/ethernet/intel/igc/igc_ethtool.c | 76 ++++++ drivers/net/ethernet/intel/igc/igc_main.c | 101 +++++++- drivers/net/ethernet/intel/igc/igc_regs.h | 16 ++ drivers/net/ethernet/intel/igc/igc_tsn.c | 219 ++++++++++++++++- drivers/net/ethernet/intel/igc/igc_tsn.h | 34 +++ drivers/net/ethernet/stmicro/stmmac/stmmac.h | 16 +- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 41 +--- .../net/ethernet/stmicro/stmmac/stmmac_fpe.c | 174 +++----------- .../net/ethernet/stmicro/stmmac/stmmac_fpe.h | 5 - .../net/ethernet/stmicro/stmmac/stmmac_main.c | 8 +- include/linux/ethtool.h | 61 +++++ net/ethtool/mm.c | 224 +++++++++++++++++- 15 files changed, 793 insertions(+), 217 deletions(-) -- 2.34.1