Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types

[Shahaf Shuler <shahafs@mellanox.com>]

Wednesday, November 14, 2018 11:41 AM, Adrien Mazarguil:
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and
> types
> 
> Hi Shahaf,
> 
> On Tue, Nov 13, 2018 at 06:39:04PM +0000, Shahaf Shuler wrote:
> > Hi Adrien,
> >
> > Tuesday, November 13, 2018 7:15 PM, Adrien Mazarguil:
> > > Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key
> > > and types
> > >
> > > Again a bit late to the party, please see below.
> > >
> > > On Sun, Nov 11, 2018 at 09:35:22AM +0000, Ori Kam wrote:
> >
> > [...]
> >
> > > > > The setfault is the result of commit a4391f8bae ("app/testpmd:
> > > > > set default RSS key as null").
> > > > > Reverting this commit should fix the segfault but it also means
> > > > > there is no way to set default key (key=NULL) with testpmd.
> > > > > Need to check if this is only a testpmd limitation and not all
> > > > > applications limitation.
> > > > >
> > > > > We should decide how an application can set default RSS without
> > > > > knowing anything about keys.
> > > > >
> > > >
> > > > I agree with Adrian that the main criteria should be the length.
> > > > Maybe the set default RSS in testpmd should get new parameter.
> > >
> > > Since [1] was reverted and we seem to agree that a zero key_len
> > > should trigger a PMD-specific default key, this can already be
> > > requested with testpmd by overriding key_len, e.g.:
> > >
> > >  flow create 1 pattern eth / end actions rss key_len 0 / end
> > >
> > > Using an empty string as the key would yield the same result but
> > > cannot be expressed on the command line yet. Note that specifying a
> > > key automatically overrides key_len, so key_len must be forced to 0 last
> to get PMD defaults:
> > >
> > >  flow create 1 pattern eth / end actions rss key foo key_len 0 / end
> >
> > I don't understand why we are backing up API claims with "how testpmd is
> implemented". The APIs should be correct, regardless of how testpmd is
> using them.
> 
> This wasn't the intent, I mean, currently one cannot input something like that
> to get a zero key length:
> 
>  flow create 1 pattern eth / end actions rss key "" / end
> 
> Because "" is interpreted literally. So the only way to request a zero key
> length is by explicitly setting it through "key_len 0".
> 
> The API remains clear: a zero key length requests default behavior from the
> PMD regardless of the key pointer, which doesn't *have* to be NULL, merely
> undefined. Testpmd does exactly that.

IMO, it will make it more clear if the key will *have* to be null, because there is no single good reason to have it otherwise. 

However it looks like an endless debate between strict and relaxed API. there are points to both sides, yet we are failing to converge.
Mlx5 already implements according to the rss_key_len and rss_key approach. What are other PMD doing?

Assuming there is a consensus among the PMDs, maybe we can follow it in order to avoid the extra work.
is it that critical for you to enforce only the key_len w/o the rss_key? 

> 
> > To this doc issue,
> > I don't understand on what cases it makes sense for application to have
> rss_key_len = 0 while rss_key != NULL. This is obviously in-consist input, and
> of course all PMD will just ignore the key.
> > I think enforcing rss_key and rss_key_len to be NULL is a fair requirement
> from application, and it makes no confusion in the API inputs.
> 
> Then you need to define what happens when key_len != 0 and key == NULL,
> also when key_len == 0 and key != NULL, none of which make sense
> currently.

Of course all are in-consist and the PMD is free to reject such rules. This is not related though to how we define the default RSS right? 

> 
> There's no reason for the PMD to even look at the key pointer if key_len is 0.
> Only if nonzero, it *can* check for its validity however there's no reason to,
> it's a programming error in the application if not the case.
> assert() is more appropriate for such situations.
> 
> I agree there's a lack of documentation which must be addressed, my point is
> that key_len is the only guarantee a PMD needs from the application.
> 
> > > Here key_len is set to testpmd's default size when parsing "rss",
> > > updated to
> > > 3 when parsing "key foo" and updated once again when parsing "key_len
> 0".
> > >
> > > Lastly, while it would make sense for testpmd to use 0 as the
> > > default value, doing so yields inconsistent balancing results
> > > between vendors/devices as they all come with a different key. Same
> > > reason as initializing the RSS types field to the global rss_hf instead of 0.
> >
> >
> >
> > >
> > > [1] "app/testpmd: revert setting default RSS"
> > >
> > >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fm
> > > a
> > > ils.dpdk.org%2Farchives%2Fdev%2F2018-
> > >
> November%2F118786.html&data=02%7C01%7Cshahafs%40mellanox.co
> > >
> m%7C0eecf3e9af4b4b6bc53108d6498ba2a8%7Ca652971c7d2e4d9ba6a4d1492
> > >
> 56f461b%7C0%7C0%7C636777261425388073&sdata=Hu0iGr2xS%2FI%2FI
> > > s5PtzCylMMft5w5TBmtd3GYppEKKcA%3D&reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND