DPDK patches and discussions
 help / color / mirror / Atom feed
From: Shahaf Shuler <shahafs@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Ori Kam <orika@mellanox.com>, Ophir Munk <ophirmu@mellanox.com>,
	Yongseok Koh <yskoh@mellanox.com>,
	Andrew Rybchenko <arybchenko@solarflare.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Thomas Monjalon <thomas@monjalon.net>,
	Asaf Penso <asafp@mellanox.com>, Olga Shern <olgas@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: document RSS default key and types
Date: Wed, 14 Nov 2018 13:51:19 +0000	[thread overview]
Message-ID: <DB7PR05MB44263A9B02BAD6FA5635E29CC3C30@DB7PR05MB4426.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20181114094040.GG17131@6wind.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&amp;data=02%7C01%7Cshahafs%40mellanox.co
> > >
> m%7C0eecf3e9af4b4b6bc53108d6498ba2a8%7Ca652971c7d2e4d9ba6a4d1492
> > >
> 56f461b%7C0%7C0%7C636777261425388073&amp;sdata=Hu0iGr2xS%2FI%2FI
> > > s5PtzCylMMft5w5TBmtd3GYppEKKcA%3D&amp;reserved=0
> 
> --
> Adrien Mazarguil
> 6WIND

  reply	other threads:[~2018-11-14 13:51 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-03 15:46 [dpdk-dev] [PATCH v1] " Ophir Munk
2018-11-05 13:11 ` Ferruh Yigit
2018-11-05 21:47 ` Thomas Monjalon
2018-11-07  9:23 ` [dpdk-dev] [PATCH v2] " Ophir Munk
2018-11-07  9:31   ` Adrien Mazarguil
2018-11-07 12:39     ` Ophir Munk
2018-11-07 14:06       ` Adrien Mazarguil
2018-11-07 15:13         ` Ophir Munk
2018-11-07 16:41           ` Adrien Mazarguil
2018-11-08  6:32             ` Andrew Rybchenko
2018-11-08  8:50             ` Ophir Munk
2018-11-08 23:07               ` Yongseok Koh
2018-11-09  8:13                 ` Ophir Munk
2018-11-11  9:35                   ` Ori Kam
2018-11-13 17:15                     ` Adrien Mazarguil
2018-11-13 18:04                       ` Ophir Munk
2018-11-13 18:39                       ` Shahaf Shuler
2018-11-14  9:40                         ` Adrien Mazarguil
2018-11-14 13:51                           ` Shahaf Shuler [this message]
2018-11-14 15:16                             ` Adrien Mazarguil

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=DB7PR05MB44263A9B02BAD6FA5635E29CC3C30@DB7PR05MB4426.eurprd05.prod.outlook.com \
    --to=shahafs@mellanox.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=arybchenko@solarflare.com \
    --cc=asafp@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=olgas@mellanox.com \
    --cc=ophirmu@mellanox.com \
    --cc=orika@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=yskoh@mellanox.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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).