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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Wed, Nov 14, 2018 at 01:51:19PM +0000, Shahaf Shuler wrote:
<snip>
> 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? 

Not at all, I don't mind extra checks in PMDs actually.

To be clear, here's a list of what I consider valid PMD checks:

- if (!key_len) use_default_key();

- if (key_len) { assert(key); use_app_key(); }

- if (key_len) { if (!key) complain(); else use_app_key(); }

- if (key_len) { if (!key) { complain(); use_default_key(); } else use_app_key(); }

- if (key && key_len) use_app_key(); else use_default_key(); /* it's OK
  since the alternative is a crash */

- if (!key_len || !key) use_default_key(); /* ditto */

While those are invalid:

- if (!key_len && !key) use_default_key(); /* err, else what? */

- if (!key_len) { assert(!key); use_default_key(); } /* unless you hate
  users */

- if (!key_len) { if (key) complain(); use_default_key(); } /* extra noise
  can be annoying */

What I'm most concerned with is rte_flow API documentation, that is, what we
ask users to do in order to achieve something. A default behavior for a zero
value is currently documented on "level" and "types" fields. "key_len" and
"queue_num" are currently lacking this information.

Just like "key", requesting RSS without providing a list of queues should
default to all configured Rx queues for convenience. In that case the
"queue" pointer can be undefined, not necessarily NULL because the PMD won't
look for queues if that list anyway, its value doesn't matter.

So documentation will describe that if "key_len" or "queue_num" are nonzero,
non-default behavior is requested and the related "key"/"queue" fields must
be set and valid. Otherwise they can remain undefined.

> > > 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? 

If we force users to set both key_len = 0 *and* key = NULL like you suggest
in order to get the default behavior and key_len is not enough, I think it
makes sense to also describe these two cases for consistency. Users are
going to wonder why is the damn PMD reading key at all if its length is
zero, so we have to explain that no, the PMD doesn't do that but the pointer
still has to be NULL anyway.

Likewise users shouldn't be tempted to enter a nonzero key length without a
valid key. It also has to be described should we choose this approach.

-- 
Adrien Mazarguil
6WIND