From:   Hariprasad Kelam <hkelam@marvell.com>
To:     Maxim Mikityanskiy <maxtram95@gmail.com>
CC:     Jakub Kicinski <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "edumazet@google.com" <edumazet@google.com>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        Linu Cherian <lcherian@marvell.com>,
        Geethasowjanya Akula <gakula@marvell.com>,
        Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@marvell.com>,
        "jhs@mojatatu.com" <jhs@mojatatu.com>,
        "xiyou.wangcong@gmail.com" <xiyou.wangcong@gmail.com>,
        "jiri@resnulli.us" <jiri@resnulli.us>,
        "saeedm@nvidia.com" <saeedm@nvidia.com>,
        "richardcochran@gmail.com" <richardcochran@gmail.com>,
        "tariqt@nvidia.com" <tariqt@nvidia.com>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "hariprasad.netdev@gmail.com" <hariprasad.netdev@gmail.com>,
        Naveen Mamindlapalli <naveenm@marvell.com>
Subject: Re: [net-next Patch v2 4/5] octeontx2-pf: Add devlink support to
 configure TL1 RR_PRIO
Thread-Topic: [net-next Patch v2 4/5] octeontx2-pf: Add devlink support to
 configure TL1 RR_PRIO
Thread-Index: AQHZMan1TsAbEnDcTkCan9gbCeCC4A==
Date:   Thu, 26 Jan 2023 17:16:42 +0000
Message-ID: <PH0PR18MB44743FF147E3207CEDA58270DECF9@PH0PR18MB4474.namprd18.prod.outlook.com>
References: <Y8hYlYk/7FfGdfy8@mail.gmail.com>
 <PH0PR18MB4474FCEAC4FA5907CAC17011DEC59@PH0PR18MB4474.namprd18.prod.outlook.com>
 <Y8qZNhUgsdOMavC4@mail.gmail.com>
 <PH0PR18MB4474DBEF155EFA4DA6BA5B10DEC59@PH0PR18MB4474.namprd18.prod.outlook.com>
 <Y803rePcLc97CGik@mail.gmail.com>
 <PH0PR18MB44741D5EBBD7B4010C78C7DFDEC89@PH0PR18MB4474.namprd18.prod.outlook.com>
 <Y87onaDuo8NkFNqC@mail.gmail.com> <20230123144548.4a2c06ae@kernel.org>
 <Y88Rug7iaC0nOGvu@mail.gmail.com>
 <PH0PR18MB44748DFCCABCDC2806A2DC2CDEC99@PH0PR18MB4474.namprd18.prod.outlook.com>
 <Y8/W5dMmkqkYFNEb@mail.gmail.com>
In-Reply-To: <Y8/W5dMmkqkYFNEb@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?VmG4uMLhvVId6N5xgYssocuu63gyfQjfBbxw5gf2XH9BGgxrUSVbi+Hs7AJc?=
 =?us-ascii?Q?rsATd+VD2ADdrV8xZPn792UjGqRcNGwcaJti/DJdE9N0/pS09AG4LQ0JFSp6?=
 =?us-ascii?Q?FKbky0nE3MDLFc3NT5Ouu1LkdwIcgVBMvz5AcuTgP6uNxOTdYd20T8wHDNDc?=
 =?us-ascii?Q?1P8ijQUc3yXZCLQuMv3JMJ4j4WoFBWbwHdO79h15mlIU5CA20hfb4eNeHP3c?=
 =?us-ascii?Q?qRrfvvr2I/0VnTvs/gcfWsJkdRdOv9dgmQHq08VN/PpxJI8iOZoskAPE+lH0?=
 =?us-ascii?Q?kV3Yp2nbsy2jtYbU/0xxm5NHaiyoe1cNcZIgdpwjN3p6aTYwtTYwx3lvjcZd?=
 =?us-ascii?Q?F00L8rxu+nTslrUY6TWrOu7tUR9EwjfAMP/9wN8zchJdxljLS37brOG8e97R?=
 =?us-ascii?Q?Biqj2+fcsCA8Dg8JN0lV7idInrW/B5zP9ijs4kVGQwNqr1UuZIUNHM5mlng0?=
 =?us-ascii?Q?Yk3xzBxE8FG9spR9xPbUQmcRCevnMGBF/CsJ84sk8g5x1kLDrQO72/4b1kqS?=
 =?us-ascii?Q?tfey5KCNEwtI2P7ozzzy8Ik3o4OJqDUTQbENOA2R6ZpFcKvz2UIXVgJJ9tKb?=
 =?us-ascii?Q?cv2P83Sorq1U9va+0NdGp9rYOvUtF0d6w0u0NKpzMyjgOht5Tv3Zbl3X0PCj?=
 =?us-ascii?Q?NckbhfIR79GaaMZGbA8YIB2aUvOZ+F0RxnhOufq6wqfW/wVaByFxzScdKCRl?=
 =?us-ascii?Q?BHaa1scT058KywtNXsOSEkX5e7DCyDvHyPlh9gurRd8rREpFmHn7j4leiALz?=
 =?us-ascii?Q?+XxJsl9mZE6rFFyM+s+VnIxC3nZgvMee5WXh1IIplS0OMhJ15djZaftCoGAS?=
 =?us-ascii?Q?amGztWsOmqC9Re1xz3xhgeSdfKa+3cVvDCWpphZrtUaXPl7tgYW2dX3/xHac?=
 =?us-ascii?Q?Svaw2RzKBwX8hkMCmx57zbMBwMj4bKt73ejLmA04ZQ73S4qiqi9syIm1FLug?=
 =?us-ascii?Q?PSSpgK5X5WMDvQWsDcohgzH/KI3iu/camw5N63y7tB1b4ugcg/f0sP5qqNu+?=
 =?us-ascii?Q?hVprYDJouzbTBugPMPYvwEkmTJ5N+cAbvuKReNja4Zyy4EHF2G/366BSbZXu?=
 =?us-ascii?Q?4yLthZumOBznmSgE8P6RuO5DGSF55OVuDpzTBF6pFg2amZaqIp2SZba/vrWN?=
 =?us-ascii?Q?BTllGf9n01Mm/FdNNmCJs6FFa5fpFM79xKUB5wJKk4kjlYq7BSo8+9vnSgZF?=
 =?us-ascii?Q?eIcoVsw/AW17HQrx5w7kNcyZk6qCky75oY3LEhIIz1bNC2SNdSZmp0HlZ9cg?=
 =?us-ascii?Q?OU9eoj33thwkw5huWDyUj8xkBhjOlqcEOX1P0Z9QHgcDg+p+w2kFgXph5E8O?=
 =?us-ascii?Q?Lg7kwXP01721RgQ6IIHhYD3X6oCu/xUN7lFQ7uWebpfiv9KioIVpRB9353J8?=
 =?us-ascii?Q?SaJUQX2qd80hx2zikJMXdi0bWR1EbcXotHbbe/MKTGfBOI931KNWJiSKmQ0Q?=
 =?us-ascii?Q?OywRSFd+m4fkuGNfAkfaF4SB4WZcGBAUb9456D8upcWVeemrUGOIbz2oTqer?=
 =?us-ascii?Q?8PcfRYuvs9P1ibAECr6GvFPjPsOOXiMbJ9ucZLyIeyjzzb9syeDJ00O6dSJH?=
 =?us-ascii?Q?80RsOq5nCcR5REpeQlSxcKdkhj+YUcNPECrCWAOw?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR18MB4474.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8f8617ac-c651-4b4e-82df-08daffc11813
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jan 2023 17:16:42.3736
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: E0nAayDsA4OIuWEd8onOcVY5I2Np0LhOpZJGjPZ7Paopa2CybONCDMYnZLdIwA0wKWj84sulSwjfJWKZXzZ5XA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR18MB3829
X-Proofpoint-GUID: QUez6QcSV2_idDSV0wGRW5Vyes2q-Bzc
X-Proofpoint-ORIG-GUID: QUez6QcSV2_idDSV0wGRW5Vyes2q-Bzc
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-26_07,2023-01-26_01,2022-06-22_01
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Tue, Jan 24, 2023 at 11:49:48AM +0000, Hariprasad Kelam wrote:
> >
> >
> > > On Mon, Jan 23, 2023 at 02:45:48PM -0800, Jakub Kicinski wrote:
> > > > On Mon, 23 Jan 2023 22:05:58 +0200 Maxim Mikityanskiy wrote:
> > > > > OK, I seem to get it now, thanks for the explanation!
> > > > >
> > > > > How do you set the priority for HTB, though? You mentioned this
> > > > > command to set priority of unclassified traffic:
> > > > >
> > > > > devlink -p dev param set pci/0002:04:00.0 name tl1_rr_prio value
> > > > > 6 \ cmode runtime
> > > > >
> > > > > But what is the command to change priority for HTB?
> > > > >
> > > > > What bothers me about using devlink to configure HTB priority is:
> > > > >
> > > > > 1. Software HTB implementation doesn't have this functionality,
> > > > > and it always prioritizes unclassified traffic. As far as I
> > > > > understand, the rule for tc stuff is that all features must have
> > > > > a reference implementation in software.
> > > > >
> > > > > 2. Adding a flag (prefer unclassified vs prefer classified) to
> > > > > HTB itself may be not straightforward, because your devlink
> > > > > command has a second purpose of setting priorities between
> > > > > PFs/VFs, and it may conflict with the HTB flag.
> > > >
> > > > If there is a two-stage hierarchy the lower level should be
> > > > controlled by devlink-rate, no?
> > >
> > > From the last picture by Hariprasad, I understood that the user sets
> > > all priorities (for unclassified traffic per PF and VF, and for HTB
> > > traffic) on the same TL2 level, i.e. it's not two-stage. (Maybe I
> > > got it all wrong again?)
> > >
> > > I asked about the command to change the HTB priority, cause the
> > > parameters aren't easily guessed, but I assume it's also devlink (i.e=
.
> > > driver-specific).
> > >
> > Currently, we don't support changing HTB priority since TC_HTB_MODIFY i=
s
> not yet supported.
> > The driver implementation is inline with htb tc framework, below are
> > commands we use for setting htb priority
> >
> > ethtool -K eth0 hw-tc-offload on
> > tc qdisc replace dev eth0 root handle 1: htb offload tc class add dev
> > eth0 parent 1: classid 1:1 htb rate 10Gbit prio 2 tc class add dev
> > eth0 parent 1: classid 1:1 htb rate 10Gbit prio 3
>=20
> I thought there was a concept of a priority of the whole HTB tree in your
> implementation...
>=20
> So, if I run these commands:
>=20
> devlink -p dev param set pci/0002:04:00.0 name tl1_rr_prio value 2 cmode
> runtime tc class add dev eth0 parent 1: classid 1:1 htb rate 10Gbit prio =
1 tc
> class add dev eth0 parent 1: classid 1:2 htb rate 10Gbit prio 3
>=20
> Will it prioritize class 1:1 over unclassified traffic, and unclassified =
traffic over
> class 1:2?
>=20
> > > If there were two levels (the upper level chooses who goes first:
> > > HTB or unclassified, and the lower level sets priorities per PF and
> > > VF for unclassified traffic), that would be more straightforward to
> > > solve: the upper level should be controlled by a new HTB parameter,
> > > and the lower level is device-specific, thus it goes to devlink.
> >
> > The PF netdev and VFs share the same physical link and share the same T=
L1
> node.
> > Hardware supports one DWRR group and the rest are strict priority
> > nodes. Driver configures the priority set by devlink to PF and VF
> > traffic TL2 nodes such that traffic is forwarded to TL1 using DWRR algo=
rithm.
> >
> > Now that if we add htb command for unclassified traffic, at any given
> > point in time HTB rule only applies to a single interface, since we
> > require to set DWRR priority at TL1, which applies to both PF/VFs, we f=
eel
> it's a different use case altogether.
>=20
> Thanks, with the example above your explanation makes sense to me now.
>=20
> So, basically, in your implementation, entities prioritized by hardware
> are: each HTB class, each VF and PF; you want to keep user-assigned
> priorities for HTB classes, you want to let the user assign a priority fo=
r
> unclassified traffic, but the latter must be equal for all VFs and PF (fo=
r DWRR
> to work), correct? And that devlink command is only useful in the HTB
> scenario, i.e. it doesn't matter what tl1_rr_prio you set if HTB is not u=
sed,
> right?
>=20
> What I don't like in the current implementation is that it adds a feature=
 to
> HTB, bypassing HTB (also not providing a software equivalent of the featu=
re).
> I would expect the priority of unclassified traffic to be configured with=
 tc
> commands that set up HTB (by the way, HTB has a "default" option to choos=
e
> a class for unclassified traffic, and a priority can be set for this clas=
s - this
> functionality can be leveraged for this purpose, or a new option can be
> added, whatever looks more appropriate). On the other hand, I understand
> your hardware limitation requiring to have the same priority for all VFs =
and PF
> on the same port.

Thanks for pointing out the "default" option. we will explore this option i=
f we can fit this into our current implementation or not and get back.
Meanwhile,   there are comments on other patches in this series and as we a=
re in line with htb offload without the devlink option we will submit
 a new version of patches addressing the comments excluding this patch.

Thanks,
Hariprasad k


>=20
> It's hard to suggest something good here, actually... An obvious thought =
is to
> let the first netdev that configures HTB choose the priority for unclassi=
fied
> traffic, and block attempts from other netdevs to set a different one, bu=
t this
> approach also has obvious drawbacks: PF has no special powers here, and i=
t
> can't set the desired priority if PF itself doesn't use HTB (or doesn't c=
onfigure
> it first, before VFs).
>=20
> Another idea of mine is to keep the devlink command for enforcement
> purpose, and make the behavior as follows:
>=20
> 1. The user will pick a priority for unclassified traffic when attaching =
HTB.
>=20
> 2. If tl1_rr_prio was set with devlink, block attempts to attach HTB with=
 a
> different default priority.
>=20
> 3. If tl1_rr_prio wasn't set or was reset, allow attaching HTB to PF with=
 any
> default priority.
>=20
> This way, VFs can't attach HTB with arbitrary priorities, only with the o=
ne that
> the admin has enforced using devlink. At the same time, if VFs aren't use=
d,
> no extra steps are needed to just use HTB on a PF. On the other hand, it =
adds
> some complexity and may sound controversial to someone. Thoughts?