[dpdk-dev] [PATCH] doc: update RSS action with best effort

[dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ori Kam]

Using the rte_flow action RSS types field,
may result in an undefined outcome.

For example selecting both UDP and TCP,
selecting TCP RSS type but the pattern is targeting UDP traffic.
another option is that the PMD doesn't support all requested types.

Until now, it wasn't clear what will happen in such cases.
This commit clarify this issue by stating that the PMD
will work in the best-effort mode.

Signed-off-by: Ori Kam <orika@mellanox.com>
---
 doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 3e5cd1e..d000560 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the underlying PMD, which depending
 on the flow rule, may result in anything ranging from empty (single queue)
 to all-inclusive RSS.
 
+Best effort will be used, in case there is a conflict inside the rule.
+The conflict can be the result of:
+
+- Conflicting RSS types, for example setting both UDP and TCP.
+
+- Conflicting between RSS types and the requested pattern to match,
+  for example matching on UDP and hashing RSS on TCP.
+
+- Hashing on types that are not supported by the PMD.
+
 Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
 overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
 field only, both can be requested simultaneously.
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/3/20 5:28 PM, Ori Kam wrote:
> Using the rte_flow action RSS types field,
> may result in an undefined outcome.
> 
> For example selecting both UDP and TCP,
> selecting TCP RSS type but the pattern is targeting UDP traffic.
> another option is that the PMD doesn't support all requested types.
> 
> Until now, it wasn't clear what will happen in such cases.
> This commit clarify this issue by stating that the PMD
> will work in the best-effort mode.
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 3e5cd1e..d000560 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the underlying PMD, which depending
>  on the flow rule, may result in anything ranging from empty (single queue)
>  to all-inclusive RSS.
>  
> +Best effort will be used, in case there is a conflict inside the rule.
> +The conflict can be the result of:
> +
> +- Conflicting RSS types, for example setting both UDP and TCP.
> +
> +- Conflicting between RSS types and the requested pattern to match,
> +  for example matching on UDP and hashing RSS on TCP.

IMHO it is not a problem at all, but good to clarify anyway.

> +
> +- Hashing on types that are not supported by the PMD.

Shouldn't it return error to the caller?

> +
>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>  field only, both can be requested simultaneously.
> 

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ori Kam]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> On 8/3/20 5:28 PM, Ori Kam wrote:
> > Using the rte_flow action RSS types field,
> > may result in an undefined outcome.
> >
> > For example selecting both UDP and TCP,
> > selecting TCP RSS type but the pattern is targeting UDP traffic.
> > another option is that the PMD doesn't support all requested types.
> >
> > Until now, it wasn't clear what will happen in such cases.
> > This commit clarify this issue by stating that the PMD
> > will work in the best-effort mode.
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index 3e5cd1e..d000560 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the
> underlying PMD, which depending
> >  on the flow rule, may result in anything ranging from empty (single queue)
> >  to all-inclusive RSS.
> >
> > +Best effort will be used, in case there is a conflict inside the rule.
> > +The conflict can be the result of:
> > +
> > +- Conflicting RSS types, for example setting both UDP and TCP.
> > +
> > +- Conflicting between RSS types and the requested pattern to match,
> > +  for example matching on UDP and hashing RSS on TCP.
> 
> IMHO it is not a problem at all, but good to clarify anyway.
> 
Agree this patch is just for clarification.

> > +
> > +- Hashing on types that are not supported by the PMD.
> 
> Shouldn't it return error to the caller?
> 
That’s depends, if for example the application requested eth and IPv4,
and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.

> > +
> >  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >  field only, both can be requested simultaneously.
> >

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/3/20 6:22 PM, Ori Kam wrote:
> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> On 8/3/20 5:28 PM, Ori Kam wrote:
>>> Using the rte_flow action RSS types field,
>>> may result in an undefined outcome.
>>>
>>> For example selecting both UDP and TCP,
>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
>>> another option is that the PMD doesn't support all requested types.
>>>
>>> Until now, it wasn't clear what will happen in such cases.
>>> This commit clarify this issue by stating that the PMD
>>> will work in the best-effort mode.
>>>
>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>> ---
>>>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>>> index 3e5cd1e..d000560 100644
>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>> @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the
>> underlying PMD, which depending
>>>  on the flow rule, may result in anything ranging from empty (single queue)
>>>  to all-inclusive RSS.
>>>
>>> +Best effort will be used, in case there is a conflict inside the rule.
>>> +The conflict can be the result of:
>>> +
>>> +- Conflicting RSS types, for example setting both UDP and TCP.
>>> +
>>> +- Conflicting between RSS types and the requested pattern to match,
>>> +  for example matching on UDP and hashing RSS on TCP.
>> IMHO it is not a problem at all, but good to clarify anyway.
>>
> Agree this patch is just for clarification.
>
>>> +
>>> +- Hashing on types that are not supported by the PMD.
>> Shouldn't it return error to the caller?
>>
> That’s depends, if for example the application requested eth and IPv4,
> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.

I think it is a bad behaviour. Application has required information to
provide correct parameters and therefore incorrect should be rejected.
Am I missing something?

>>> +
>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>>>  field only, both can be requested simultaneously.
>>>

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[David Marchand]

On Mon, Aug 3, 2020 at 5:23 PM Ori Kam <orika@mellanox.com> wrote:
> > > +
> > > +- Hashing on types that are not supported by the PMD.
> >
> > Shouldn't it return error to the caller?
> >
> That’s depends, if for example the application requested eth and IPv4,
> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.

We should have some validation in ethdev to catch this.
Is it not the case?


-- 
David Marchand

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ori Kam]

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> On 8/3/20 6:22 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>
> >> On 8/3/20 5:28 PM, Ori Kam wrote:
> >>> Using the rte_flow action RSS types field,
> >>> may result in an undefined outcome.
> >>>
> >>> For example selecting both UDP and TCP,
> >>> selecting TCP RSS type but the pattern is targeting UDP traffic.
> >>> another option is that the PMD doesn't support all requested types.
> >>>
> >>> Until now, it wasn't clear what will happen in such cases.
> >>> This commit clarify this issue by stating that the PMD
> >>> will work in the best-effort mode.
> >>>
> >>> Signed-off-by: Ori Kam <orika@mellanox.com>
> >>> ---
> >>>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >> b/doc/guides/prog_guide/rte_flow.rst
> >>> index 3e5cd1e..d000560 100644
> >>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>> @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the
> >> underlying PMD, which depending
> >>>  on the flow rule, may result in anything ranging from empty (single queue)
> >>>  to all-inclusive RSS.
> >>>
> >>> +Best effort will be used, in case there is a conflict inside the rule.
> >>> +The conflict can be the result of:
> >>> +
> >>> +- Conflicting RSS types, for example setting both UDP and TCP.
> >>> +
> >>> +- Conflicting between RSS types and the requested pattern to match,
> >>> +  for example matching on UDP and hashing RSS on TCP.
> >> IMHO it is not a problem at all, but good to clarify anyway.
> >>
> > Agree this patch is just for clarification.
> >
> >>> +
> >>> +- Hashing on types that are not supported by the PMD.
> >> Shouldn't it return error to the caller?
> >>
> > That’s depends, if for example the application requested eth and IPv4,
> > and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
> 
> I think it is a bad behaviour. Application has required information to
> provide correct parameters and therefore incorrect should be rejected.
> Am I missing something?
> 
In RTE flow you can't know what are the parameters that are supported.
One option is to fail the rule, but this means failing the rule even if some of them are supported.
Or we can choose to fail if all the types are unsupported, but this will result in miss match between
adding two types and adding just one type. For example the PMD doesn't support RSS on eth
and the application request RSS on ETH and IPv4 so the PMD will do IPv4 RSS,
while if the user selects only ETH the rule will fail.
Same this is exactly the same case as miss match the application requests something
that the PMD can't supply. 
In any case I think this is the current behavior of the PMD so this just clarify it.

> >>> +
> >>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >>>  field only, both can be requested simultaneously.
> >>>

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ori Kam]

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> 
> On Mon, Aug 3, 2020 at 5:23 PM Ori Kam <orika@mellanox.com> wrote:
> > > > +
> > > > +- Hashing on types that are not supported by the PMD.
> > >
> > > Shouldn't it return error to the caller?
> > >
> > That’s depends, if for example the application requested eth and IPv4,
> > and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
> 
> We should have some validation in ethdev to catch this.
> Is it not the case?
> 
Like I said in my reply to Andrew, in rte_flow we don't have such caps.
Maybe we should think about adding them for the RSS case, but again it is tricky
What if one RSS type is supported depending on the flow or other types?

> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/3/20 6:49 PM, Ori Kam wrote:
> Hi David,
>
>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>>
>> On Mon, Aug 3, 2020 at 5:23 PM Ori Kam <orika@mellanox.com> wrote:
>>>>> +
>>>>> +- Hashing on types that are not supported by the PMD.
>>>> Shouldn't it return error to the caller?
>>>>
>>> That’s depends, if for example the application requested eth and IPv4,
>>> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
>> We should have some validation in ethdev to catch this.
>> Is it not the case?
>>
> Like I said in my reply to Andrew, in rte_flow we don't have such caps.
> Maybe we should think about adding them for the RSS case, but again it is tricky
> What if one RSS type is supported depending on the flow or other types?

Also I'd like to add that ethdev layer is dummy for rte_flow API.
It does not parse pattern/actions etc. May be should change it one day.

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/3/20 6:47 PM, Ori Kam wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> On 8/3/20 6:22 PM, Ori Kam wrote:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>
>>>> On 8/3/20 5:28 PM, Ori Kam wrote:
>>>>> Using the rte_flow action RSS types field,
>>>>> may result in an undefined outcome.
>>>>>
>>>>> For example selecting both UDP and TCP,
>>>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
>>>>> another option is that the PMD doesn't support all requested types.
>>>>>
>>>>> Until now, it wasn't clear what will happen in such cases.
>>>>> This commit clarify this issue by stating that the PMD
>>>>> will work in the best-effort mode.
>>>>>
>>>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>>>> ---
>>>>>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>>>> b/doc/guides/prog_guide/rte_flow.rst
>>>>> index 3e5cd1e..d000560 100644
>>>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>>>> @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the
>>>> underlying PMD, which depending
>>>>>  on the flow rule, may result in anything ranging from empty (single queue)
>>>>>  to all-inclusive RSS.
>>>>>
>>>>> +Best effort will be used, in case there is a conflict inside the rule.
>>>>> +The conflict can be the result of:
>>>>> +
>>>>> +- Conflicting RSS types, for example setting both UDP and TCP.
>>>>> +
>>>>> +- Conflicting between RSS types and the requested pattern to match,
>>>>> +  for example matching on UDP and hashing RSS on TCP.
>>>> IMHO it is not a problem at all, but good to clarify anyway.
>>>>
>>> Agree this patch is just for clarification.
>>>
>>>>> +
>>>>> +- Hashing on types that are not supported by the PMD.
>>>> Shouldn't it return error to the caller?
>>>>
>>> That’s depends, if for example the application requested eth and IPv4,
>>> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
>>
>> I think it is a bad behaviour. Application has required information to
>> provide correct parameters and therefore incorrect should be rejected.
>> Am I missing something?
>>
> In RTE flow you can't know what are the parameters that are supported.
> One option is to fail the rule, but this means failing the rule even if some of them are supported.
> Or we can choose to fail if all the types are unsupported, but this will result in miss match between
> adding two types and adding just one type. For example the PMD doesn't support RSS on eth
> and the application request RSS on ETH and IPv4 so the PMD will do IPv4 RSS,
> while if the user selects only ETH the rule will fail.
> Same this is exactly the same case as miss match the application requests something
> that the PMD can't supply. 
> In any case I think this is the current behavior of the PMD so this just clarify it.

I'm sorry, but the PMD is hardly applicable here.
Do you mean the "mlx5" PMD? Or have you reviewed all PMDs?

No I understand that the topic is more complicated, but
covering complex cases should not spoil simple.
I think that information provided in dev_info should
specify all supported protocols/fields.
Yes, it is acceptable to behave best effort if requested
field is supported in general, but it not for the flow.
Please, make it a bit clearer in the suggested description.

> 
>>>>> +
>>>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>>>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>>>>>  field only, both can be requested simultaneously.
>>>>>
> 

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Thomas Monjalon]

03/08/2020 17:55, Andrew Rybchenko:
> On 8/3/20 6:49 PM, Ori Kam wrote:
> > From: David Marchand <david.marchand@redhat.com>
> >> On Mon, Aug 3, 2020 at 5:23 PM Ori Kam <orika@mellanox.com> wrote:
> >>>>> +
> >>>>> +- Hashing on types that are not supported by the PMD.
> >>>> Shouldn't it return error to the caller?
> >>>>
> >>> That’s depends, if for example the application requested eth and IPv4,
> >>> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
> >> We should have some validation in ethdev to catch this.
> >> Is it not the case?
> >>
> > Like I said in my reply to Andrew, in rte_flow we don't have such caps.
> > Maybe we should think about adding them for the RSS case, but again it is tricky
> > What if one RSS type is supported depending on the flow or other types?
> 
> Also I'd like to add that ethdev layer is dummy for rte_flow API.
> It does not parse pattern/actions etc. May be should change it one day.

For now, what we can do is to have "best effort" (sic) checks.
In the case we are 100% sure a rule cannot be fully supported,
I think we should reject the rule.
If there are more complex cases to handle, we can accept the rule
and support it with "best effort" handling.

The other appropriate action is to implement tests in DTS
to check the behaviour of the PMDs, validating the compliance
of the accepted rules with real flows.

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ori Kam]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> On 8/3/20 6:47 PM, Ori Kam wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>
> >> On 8/3/20 6:22 PM, Ori Kam wrote:
> >>> Hi Andrew,
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>
> >>>> On 8/3/20 5:28 PM, Ori Kam wrote:
> >>>>> Using the rte_flow action RSS types field,
> >>>>> may result in an undefined outcome.
> >>>>>
> >>>>> For example selecting both UDP and TCP,
> >>>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
> >>>>> another option is that the PMD doesn't support all requested types.
> >>>>>
> >>>>> Until now, it wasn't clear what will happen in such cases.
> >>>>> This commit clarify this issue by stating that the PMD
> >>>>> will work in the best-effort mode.
> >>>>>
> >>>>> Signed-off-by: Ori Kam <orika@mellanox.com>
> >>>>> ---
> >>>>>  doc/guides/prog_guide/rte_flow.rst | 10 ++++++++++
> >>>>>  1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >>>> b/doc/guides/prog_guide/rte_flow.rst
> >>>>> index 3e5cd1e..d000560 100644
> >>>>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>>>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>>>> @@ -1735,6 +1735,16 @@ unspecified "best-effort" settings from the
> >>>> underlying PMD, which depending
> >>>>>  on the flow rule, may result in anything ranging from empty (single
> queue)
> >>>>>  to all-inclusive RSS.
> >>>>>
> >>>>> +Best effort will be used, in case there is a conflict inside the rule.
> >>>>> +The conflict can be the result of:
> >>>>> +
> >>>>> +- Conflicting RSS types, for example setting both UDP and TCP.
> >>>>> +
> >>>>> +- Conflicting between RSS types and the requested pattern to match,
> >>>>> +  for example matching on UDP and hashing RSS on TCP.
> >>>> IMHO it is not a problem at all, but good to clarify anyway.
> >>>>
> >>> Agree this patch is just for clarification.
> >>>
> >>>>> +
> >>>>> +- Hashing on types that are not supported by the PMD.
> >>>> Shouldn't it return error to the caller?
> >>>>
> >>> That’s depends, if for example the application requested eth and IPv4,
> >>> and the PMD can do only IPv4 so according to this, the PMD will just do
> IPv4.
> >>
> >> I think it is a bad behaviour. Application has required information to
> >> provide correct parameters and therefore incorrect should be rejected.
> >> Am I missing something?
> >>
> > In RTE flow you can't know what are the parameters that are supported.
> > One option is to fail the rule, but this means failing the rule even if some of
> them are supported.
> > Or we can choose to fail if all the types are unsupported, but this will result in
> miss match between
> > adding two types and adding just one type. For example the PMD doesn't
> support RSS on eth
> > and the application request RSS on ETH and IPv4 so the PMD will do IPv4 RSS,
> > while if the user selects only ETH the rule will fail.
> > Same this is exactly the same case as miss match the application requests
> something
> > that the PMD can't supply.
> > In any case I think this is the current behavior of the PMD so this just clarify it.
> 
> I'm sorry, but the PMD is hardly applicable here.
> Do you mean the "mlx5" PMD? Or have you reviewed all PMDs?
> 
I assume that each PMD as its own limitations, I'm sure that no
PMD can support all possible RSS. But maybe there is such perfect PMD.

> No I understand that the topic is more complicated, but
> covering complex cases should not spoil simple.
> I think that information provided in dev_info should
> specify all supported protocols/fields.
> Yes, it is acceptable to behave best effort if requested
> field is supported in general, but it not for the flow.
> Please, make it a bit clearer in the suggested description.
> 
So what about the following wording: (after the current part)
If requested RSS hash type is not supported, and no supported hash function
is requested then the PMD may reject the flow.

> >
> >>>>> +
> >>>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >>>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >>>>>  field only, both can be requested simultaneously.
> >>>>>
> >

[dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Ori Kam]

Using the rte_flow action RSS types field,
may result in undefined outcome.

For example selecting both UDP and TCP,
selecting TCP RSS type but the pattern is targeting UDP traffic.
another option is that the PMD doesn't support all requested types.

Until now, it wasn't clear what will happen in such cases.
This commit clarify this issue by stating that the PMD
will work in the best-effort mode.

Signed-off-by: Ori Kam <orika@mellanox.com>
---
v2:
 * Based on ML, update that using only unsupported hash type
   may cause the flow to be rejected by PMD.
---
 doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 3e5cd1e..d730b66 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the underlying PMD, which depending
 on the flow rule, may result in anything ranging from empty (single queue)
 to all-inclusive RSS.
 
+Best effort will be used, in case there is a conflict inside the rule.
+The conflict can be the result of:
+
+- Conflicting RSS types, for example setting both UDP and TCP.
+
+- Conflicting between RSS types and the requested pattern to match,
+  for example matching on UDP and hashing RSS on TCP.
+
+If requested RSS hash type is not supported,
+and no supported hash type is requested then the PMD may reject the flow.
+
 Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
 overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
 field only, both can be requested simultaneously.
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Ori Kam]

Hi All,

Can you please review and ack it? so it will be merged in 20.08 version.

Thanks,
Ori

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> 
> Using the rte_flow action RSS types field,
> may result in undefined outcome.
> 
> For example selecting both UDP and TCP,
> selecting TCP RSS type but the pattern is targeting UDP traffic.
> another option is that the PMD doesn't support all requested types.
> 
> Until now, it wasn't clear what will happen in such cases.
> This commit clarify this issue by stating that the PMD
> will work in the best-effort mode.
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
> v2:
>  * Based on ML, update that using only unsupported hash type
>    may cause the flow to be rejected by PMD.
> ---
>  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> index 3e5cd1e..d730b66 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the
> underlying PMD, which depending
>  on the flow rule, may result in anything ranging from empty (single queue)
>  to all-inclusive RSS.
> 
> +Best effort will be used, in case there is a conflict inside the rule.
> +The conflict can be the result of:
> +
> +- Conflicting RSS types, for example setting both UDP and TCP.
> +
> +- Conflicting between RSS types and the requested pattern to match,
> +  for example matching on UDP and hashing RSS on TCP.
> +
> +If requested RSS hash type is not supported,
> +and no supported hash type is requested then the PMD may reject the flow.
> +
>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>  field only, both can be requested simultaneously.
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/4/20 11:13 AM, Ori Kam wrote:
> Using the rte_flow action RSS types field,
> may result in undefined outcome.
> 
> For example selecting both UDP and TCP,
> selecting TCP RSS type but the pattern is targeting UDP traffic.
> another option is that the PMD doesn't support all requested types.
> 
> Until now, it wasn't clear what will happen in such cases.
> This commit clarify this issue by stating that the PMD
> will work in the best-effort mode.
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
> v2:
>  * Based on ML, update that using only unsupported hash type
>    may cause the flow to be rejected by PMD.
> ---
>  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 3e5cd1e..d730b66 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the underlying PMD, which depending
>  on the flow rule, may result in anything ranging from empty (single queue)
>  to all-inclusive RSS.
>  
> +Best effort will be used, in case there is a conflict inside the rule.
> +The conflict can be the result of:
> +
> +- Conflicting RSS types, for example setting both UDP and TCP.

Thinking more about it, I see no conflict at all. It is common
to specify both TCP and UDP in hash function if user wants to
take TCP ports for TCP packets and UDP ports for UDP into
account.

> +
> +- Conflicting between RSS types and the requested pattern to match,
> +  for example matching on UDP and hashing RSS on TCP.

I'd not name it a conflict either. It is just common rule when
applicable fields are used only.

> +
> +If requested RSS hash type is not supported,
> +and no supported hash type is requested then the PMD may reject the flow.
> +

I disagree with such description. If requested RSS hash type is
not supported (not present in dev_info.flow_type_rss_offloads),
it must be rejected and error returned.
If requested RSS hash type is not supported for a packet
matching the rule, but supported in general (present in
dev_info.flow_type_rss_offloads), part of RSS hash type
specification may be ignored and only applicable bits are used.
If result is empty, PMD may reject the flow rule.

>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>  field only, both can be requested simultaneously.
> 

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Ori Kam]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> On 8/4/20 11:13 AM, Ori Kam wrote:
> > Using the rte_flow action RSS types field,
> > may result in undefined outcome.
> >
> > For example selecting both UDP and TCP,
> > selecting TCP RSS type but the pattern is targeting UDP traffic.
> > another option is that the PMD doesn't support all requested types.
> >
> > Until now, it wasn't clear what will happen in such cases.
> > This commit clarify this issue by stating that the PMD
> > will work in the best-effort mode.
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> > v2:
> >  * Based on ML, update that using only unsupported hash type
> >    may cause the flow to be rejected by PMD.
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index 3e5cd1e..d730b66 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the
> underlying PMD, which depending
> >  on the flow rule, may result in anything ranging from empty (single queue)
> >  to all-inclusive RSS.
> >
> > +Best effort will be used, in case there is a conflict inside the rule.
> > +The conflict can be the result of:
> > +
> > +- Conflicting RSS types, for example setting both UDP and TCP.
> 
> Thinking more about it, I see no conflict at all. It is common
> to specify both TCP and UDP in hash function if user wants to
> take TCP ports for TCP packets and UDP ports for UDP into
> account.
> 
I fully agree with you that it is common to use both UDP and TCP and
expect it to work based on the traffic. this is the point of this patch.
To clarify that this is valid input and the PMD will work in best effort mode.

> > +
> > +- Conflicting between RSS types and the requested pattern to match,
> > +  for example matching on UDP and hashing RSS on TCP.
> 
> I'd not name it a conflict either. It is just common rule when
> applicable fields are used only.
> 
Just like my comment from above.

> > +
> > +If requested RSS hash type is not supported,
> > +and no supported hash type is requested then the PMD may reject the flow.
> > +
> 
> I disagree with such description. If requested RSS hash type is
> not supported (not present in dev_info.flow_type_rss_offloads),
> it must be rejected and error returned.
> If requested RSS hash type is not supported for a packet
> matching the rule, but supported in general (present in
> dev_info.flow_type_rss_offloads), part of RSS hash type
> specification may be ignored and only applicable bits are used.
> If result is empty, PMD may reject the flow rule.
> 
The flow should be rejected even if it is used with some type that is supported?


> >  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >  field only, both can be requested simultaneously.
> >

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/5/20 5:08 PM, Ori Kam wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> On 8/4/20 11:13 AM, Ori Kam wrote:
>>> Using the rte_flow action RSS types field,
>>> may result in undefined outcome.
>>>
>>> For example selecting both UDP and TCP,
>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
>>> another option is that the PMD doesn't support all requested types.
>>>
>>> Until now, it wasn't clear what will happen in such cases.
>>> This commit clarify this issue by stating that the PMD
>>> will work in the best-effort mode.
>>>
>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>> ---
>>> v2:
>>>  * Based on ML, update that using only unsupported hash type
>>>    may cause the flow to be rejected by PMD.
>>> ---
>>>  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>>> index 3e5cd1e..d730b66 100644
>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>> @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the
>> underlying PMD, which depending
>>>  on the flow rule, may result in anything ranging from empty (single queue)
>>>  to all-inclusive RSS.
>>>
>>> +Best effort will be used, in case there is a conflict inside the rule.
>>> +The conflict can be the result of:
>>> +
>>> +- Conflicting RSS types, for example setting both UDP and TCP.
>>
>> Thinking more about it, I see no conflict at all. It is common
>> to specify both TCP and UDP in hash function if user wants to
>> take TCP ports for TCP packets and UDP ports for UDP into
>> account.
>>
> I fully agree with you that it is common to use both UDP and TCP and
> expect it to work based on the traffic. this is the point of this patch.
> To clarify that this is valid input and the PMD will work in best effort mode.

Just don't name it "Conflicting RSS types", may be
"Non-applicable RSS types", but it collapses two
bullets into one.

> 
>>> +
>>> +- Conflicting between RSS types and the requested pattern to match,
>>> +  for example matching on UDP and hashing RSS on TCP.
>>
>> I'd not name it a conflict either. It is just common rule when
>> applicable fields are used only.
>>
> Just like my comment from above.
> 
>>> +
>>> +If requested RSS hash type is not supported,
>>> +and no supported hash type is requested then the PMD may reject the flow.
>>> +
>>
>> I disagree with such description. If requested RSS hash type is
>> not supported (not present in dev_info.flow_type_rss_offloads),
>> it must be rejected and error returned.
>> If requested RSS hash type is not supported for a packet
>> matching the rule, but supported in general (present in
>> dev_info.flow_type_rss_offloads), part of RSS hash type
>> specification may be ignored and only applicable bits are used.
>> If result is empty, PMD may reject the flow rule.
>>
> The flow should be rejected even if it is used with some type that is supported?

Yes, if user ask for something what is not supported at all,
it is a problem and user should be notified. May be it is
too restrictive, but I'd prefer this way.
User has all required information in dev_info and can
remove unsupported hash types from request. IMHO, it
should enforce user check the result to be not empty and
handle it properly.

> 
> 
>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>>>  field only, both can be requested simultaneously.
>>>
> 

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Ori Kam]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> On 8/5/20 5:08 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>
> >> On 8/4/20 11:13 AM, Ori Kam wrote:
> >>> Using the rte_flow action RSS types field,
> >>> may result in undefined outcome.
> >>>
> >>> For example selecting both UDP and TCP,
> >>> selecting TCP RSS type but the pattern is targeting UDP traffic.
> >>> another option is that the PMD doesn't support all requested types.
> >>>
> >>> Until now, it wasn't clear what will happen in such cases.
> >>> This commit clarify this issue by stating that the PMD
> >>> will work in the best-effort mode.
> >>>
> >>> Signed-off-by: Ori Kam <orika@mellanox.com>
> >>> ---
> >>> v2:
> >>>  * Based on ML, update that using only unsupported hash type
> >>>    may cause the flow to be rejected by PMD.
> >>> ---
> >>>  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >> b/doc/guides/prog_guide/rte_flow.rst
> >>> index 3e5cd1e..d730b66 100644
> >>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>> @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the
> >> underlying PMD, which depending
> >>>  on the flow rule, may result in anything ranging from empty (single queue)
> >>>  to all-inclusive RSS.
> >>>
> >>> +Best effort will be used, in case there is a conflict inside the rule.
> >>> +The conflict can be the result of:
> >>> +
> >>> +- Conflicting RSS types, for example setting both UDP and TCP.
> >>
> >> Thinking more about it, I see no conflict at all. It is common
> >> to specify both TCP and UDP in hash function if user wants to
> >> take TCP ports for TCP packets and UDP ports for UDP into
> >> account.
> >>
> > I fully agree with you that it is common to use both UDP and TCP and
> > expect it to work based on the traffic. this is the point of this patch.
> > To clarify that this is valid input and the PMD will work in best effort mode.
> 
> Just don't name it "Conflicting RSS types", may be
> "Non-applicable RSS types", but it collapses two
> bullets into one.

Will rephrase.
> 
> >
> >>> +
> >>> +- Conflicting between RSS types and the requested pattern to match,
> >>> +  for example matching on UDP and hashing RSS on TCP.
> >>
> >> I'd not name it a conflict either. It is just common rule when
> >> applicable fields are used only.
> >>
> > Just like my comment from above.
> >
> >>> +
> >>> +If requested RSS hash type is not supported,
> >>> +and no supported hash type is requested then the PMD may reject the
> flow.
> >>> +
> >>
> >> I disagree with such description. If requested RSS hash type is
> >> not supported (not present in dev_info.flow_type_rss_offloads),
> >> it must be rejected and error returned.
> >> If requested RSS hash type is not supported for a packet
> >> matching the rule, but supported in general (present in
> >> dev_info.flow_type_rss_offloads), part of RSS hash type
> >> specification may be ignored and only applicable bits are used.
> >> If result is empty, PMD may reject the flow rule.
> >>
> > The flow should be rejected even if it is used with some type that is
> supported?
> 
> Yes, if user ask for something what is not supported at all,
> it is a problem and user should be notified. May be it is
> too restrictive, but I'd prefer this way.
> User has all required information in dev_info and can
> remove unsupported hash types from request. IMHO, it
> should enforce user check the result to be not empty and
> handle it properly.
> 
Will update the doc.
> >
> >
> >>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >>>  field only, both can be requested simultaneously.
> >>>
> >

[dpdk-dev] [PATCH v3] doc: update RSS action with best effort

[Ori Kam]

Using the rte_flow action RSS types field,
may result in undefined outcome.

For example selecting both UDP and TCP,
selecting TCP RSS type but the pattern is targeting UDP traffic.
another option is that the PMD doesn't support all requested types.

Until now, it wasn't clear what will happen in such cases.
This commit clarify this issue by stating that the PMD
will work in the best-effort mode, and will fail
in case the requested type is not supported.

Signed-off-by: Ori Kam <orika@mellanox.com>
---
v3:
 * Address ML comments.

v2:
 * Address ML comments.
---
 doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 3e5cd1e..eada283 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1735,6 +1735,18 @@ unspecified "best-effort" settings from the underlying PMD, which depending
 on the flow rule, may result in anything ranging from empty (single queue)
 to all-inclusive RSS.
 
+In case there are Non-applicable RSS types in the rule,
+Best effort will be used.
+This may be the result of:
+
+- Setting both UDP and TCP on the same rule.
+
+- Setting RSS types that don't match the requested pattern,
+  for example, matching on UDP and hashing RSS on TCP.
+
+If requested RSS hash type is not supported in ``dev_info``.
+The flow creation will fail.
+
 Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
 overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
 field only, both can be requested simultaneously.
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/7/20 8:02 AM, Ori Kam wrote:
> Using the rte_flow action RSS types field,
> may result in undefined outcome.
> 
> For example selecting both UDP and TCP,
> selecting TCP RSS type but the pattern is targeting UDP traffic.
> another option is that the PMD doesn't support all requested types.
> 
> Until now, it wasn't clear what will happen in such cases.
> This commit clarify this issue by stating that the PMD
> will work in the best-effort mode, and will fail
> in case the requested type is not supported.
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
> v3:
>  * Address ML comments.
> 
> v2:
>  * Address ML comments.
> ---
>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 3e5cd1e..eada283 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1735,6 +1735,18 @@ unspecified "best-effort" settings from the underlying PMD, which depending
>  on the flow rule, may result in anything ranging from empty (single queue)
>  to all-inclusive RSS.
>  
> +In case there are Non-applicable RSS types in the rule,
> +Best effort will be used.
> +This may be the result of:
> +
> +- Setting both UDP and TCP on the same rule.
> +
> +- Setting RSS types that don't match the requested pattern,
> +  for example, matching on UDP and hashing RSS on TCP.
> +
> +If requested RSS hash type is not supported in ``dev_info``.
> +The flow creation will fail.
> +

Consider:
If non-applicable for matching packets RSS types are requested,
these RSS types are simply ignored. For example, it happens if:

  - Hashing of both TCP and UDP ports is requested
    (only one can present in a packet).

  - Requested RSS types contradict to flow rule pattern
    (e.g. pattern has UDP item, but RSS types contain TCP).

If requested RSS types are not supported by the Ethernet device
at all (not reported in ``dev_info.flow_type_rss_offloads``),
the flow creation will fail.

>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>  field only, both can be requested simultaneously.
> 

Re: [dpdk-dev] [PATCH v3] doc: update RSS action with best effort

[Ori Kam]

Hi Andrew,


> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> On 8/7/20 8:02 AM, Ori Kam wrote:
> > Using the rte_flow action RSS types field,
> > may result in undefined outcome.
> >
> > For example selecting both UDP and TCP,
> > selecting TCP RSS type but the pattern is targeting UDP traffic.
> > another option is that the PMD doesn't support all requested types.
> >
> > Until now, it wasn't clear what will happen in such cases.
> > This commit clarify this issue by stating that the PMD
> > will work in the best-effort mode, and will fail
> > in case the requested type is not supported.
> >
> > Signed-off-by: Ori Kam <orika@mellanox.com>
> > ---
> > v3:
> >  * Address ML comments.
> >
> > v2:
> >  * Address ML comments.
> > ---
> >  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index 3e5cd1e..eada283 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1735,6 +1735,18 @@ unspecified "best-effort" settings from the
> underlying PMD, which depending
> >  on the flow rule, may result in anything ranging from empty (single queue)
> >  to all-inclusive RSS.
> >
> > +In case there are Non-applicable RSS types in the rule,
> > +Best effort will be used.
> > +This may be the result of:
> > +
> > +- Setting both UDP and TCP on the same rule.
> > +
> > +- Setting RSS types that don't match the requested pattern,
> > +  for example, matching on UDP and hashing RSS on TCP.
> > +
> > +If requested RSS hash type is not supported in ``dev_info``.
> > +The flow creation will fail.
> > +
> 
> Consider:
> If non-applicable for matching packets RSS types are requested,
> these RSS types are simply ignored. For example, it happens if:
> 
>   - Hashing of both TCP and UDP ports is requested
>     (only one can present in a packet).
> 
>   - Requested RSS types contradict to flow rule pattern
>     (e.g. pattern has UDP item, but RSS types contain TCP).
> 
> If requested RSS types are not supported by the Ethernet device
> at all (not reported in ``dev_info.flow_type_rss_offloads``),
> the flow creation will fail.
> 
Sure sounds good, but since this is just copying your words, 
I will add your Signed-off-by to this patch, is that O.K?

> >  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
> >  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
> >  field only, both can be requested simultaneously.
> >

Re: [dpdk-dev] [PATCH v3] doc: update RSS action with best effort

[Andrew Rybchenko]

On 8/10/20 10:40 AM, Ori Kam wrote:
> Hi Andrew,
>
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> On 8/7/20 8:02 AM, Ori Kam wrote:
>>> Using the rte_flow action RSS types field,
>>> may result in undefined outcome.
>>>
>>> For example selecting both UDP and TCP,
>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
>>> another option is that the PMD doesn't support all requested types.
>>>
>>> Until now, it wasn't clear what will happen in such cases.
>>> This commit clarify this issue by stating that the PMD
>>> will work in the best-effort mode, and will fail
>>> in case the requested type is not supported.
>>>
>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>> ---
>>> v3:
>>>  * Address ML comments.
>>>
>>> v2:
>>>  * Address ML comments.
>>> ---
>>>  doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>>> index 3e5cd1e..eada283 100644
>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>> @@ -1735,6 +1735,18 @@ unspecified "best-effort" settings from the
>> underlying PMD, which depending
>>>  on the flow rule, may result in anything ranging from empty (single queue)
>>>  to all-inclusive RSS.
>>>
>>> +In case there are Non-applicable RSS types in the rule,
>>> +Best effort will be used.
>>> +This may be the result of:
>>> +
>>> +- Setting both UDP and TCP on the same rule.
>>> +
>>> +- Setting RSS types that don't match the requested pattern,
>>> +  for example, matching on UDP and hashing RSS on TCP.
>>> +
>>> +If requested RSS hash type is not supported in ``dev_info``.
>>> +The flow creation will fail.
>>> +
>> Consider:
>> If non-applicable for matching packets RSS types are requested,
>> these RSS types are simply ignored. For example, it happens if:
>>
>>   - Hashing of both TCP and UDP ports is requested
>>     (only one can present in a packet).
>>
>>   - Requested RSS types contradict to flow rule pattern
>>     (e.g. pattern has UDP item, but RSS types contain TCP).
>>
>> If requested RSS types are not supported by the Ethernet device
>> at all (not reported in ``dev_info.flow_type_rss_offloads``),
>> the flow creation will fail.
>>
> Sure sounds good, but since this is just copying your words, 
> I will add your Signed-off-by to this patch, is that O.K?

OK

>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>>>  field only, both can be requested simultaneously.
>>>

[dpdk-dev] [PATCH v4] doc: update RSS action with best effort

[Ori Kam]

Using the rte_flow action RSS types field,
may result in undefined outcome.

For example selecting both UDP and TCP,
selecting TCP RSS type but the pattern is targeting UDP traffic.
another option is that the PMD doesn't support all requested types.

Until now, it wasn't clear what will happen in such cases.
This commit clarify this issue by stating that the PMD
will work in the best-effort mode, and will fail
in case the requested type is not supported.

Signed-off-by: Ori Kam <orika@nvidia.com>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/prog_guide/rte_flow.rst | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 3e5cd1e..944e824 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1735,6 +1735,19 @@ unspecified "best-effort" settings from the underlying PMD, which depending
 on the flow rule, may result in anything ranging from empty (single queue)
 to all-inclusive RSS.
 
+If non-applicable for matching packets RSS types are requested,
+these RSS types are simply ignored. For example, it happens if:
+
+- Hashing of both TCP and UDP ports is requested
+  (only one can present in a packet).
+
+- Requested RSS types contradict to flow rule pattern
+  (e.g. pattern has UDP item, but RSS types contain TCP).
+
+If requested RSS hash types are not supported by the Ethernet device at all
+(not reported in ``dev_info.flow_tpe_rss_offloads``),
+the flow creation will fail.
+
 Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
 overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
 field only, both can be requested simultaneously.
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] doc: update RSS action with best effort

[Ferruh Yigit]

On 8/3/2020 4:55 PM, Andrew Rybchenko wrote:
> On 8/3/20 6:49 PM, Ori Kam wrote:
>> Hi David,
>>
>>> -----Original Message-----
>>> From: David Marchand <david.marchand@redhat.com>
>>>
>>> On Mon, Aug 3, 2020 at 5:23 PM Ori Kam <orika@mellanox.com> wrote:
>>>>>> +
>>>>>> +- Hashing on types that are not supported by the PMD.
>>>>> Shouldn't it return error to the caller?
>>>>>
>>>> That’s depends, if for example the application requested eth and IPv4,
>>>> and the PMD can do only IPv4 so according to this, the PMD will just do IPv4.
>>> We should have some validation in ethdev to catch this.
>>> Is it not the case?
>>>
>> Like I said in my reply to Andrew, in rte_flow we don't have such caps.
>> Maybe we should think about adding them for the RSS case, but again it is tricky
>> What if one RSS type is supported depending on the flow or other types?
> 
> Also I'd like to add that ethdev layer is dummy for rte_flow API.
> It does not parse pattern/actions etc. May be should change it one day.
> 

As far as I remember not having capabilities was the design decision,
PMD does the validation via 'rte_flow_validate()'.

Indeed it can be quite complex to have that kind of capability
reporting, and without something like that there is not much to do in
ethdev layer.

Re: [dpdk-dev] [PATCH v2] doc: update RSS action with best effort

[Ferruh Yigit]

On 8/5/2020 3:08 PM, Ori Kam wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> On 8/4/20 11:13 AM, Ori Kam wrote:
>>> Using the rte_flow action RSS types field,
>>> may result in undefined outcome.
>>>
>>> For example selecting both UDP and TCP,
>>> selecting TCP RSS type but the pattern is targeting UDP traffic.
>>> another option is that the PMD doesn't support all requested types.
>>>
>>> Until now, it wasn't clear what will happen in such cases.
>>> This commit clarify this issue by stating that the PMD
>>> will work in the best-effort mode.
>>>
>>> Signed-off-by: Ori Kam <orika@mellanox.com>
>>> ---
>>> v2:
>>>  * Based on ML, update that using only unsupported hash type
>>>    may cause the flow to be rejected by PMD.
>>> ---
>>>  doc/guides/prog_guide/rte_flow.rst | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
>> b/doc/guides/prog_guide/rte_flow.rst
>>> index 3e5cd1e..d730b66 100644
>>> --- a/doc/guides/prog_guide/rte_flow.rst
>>> +++ b/doc/guides/prog_guide/rte_flow.rst
>>> @@ -1735,6 +1735,17 @@ unspecified "best-effort" settings from the
>> underlying PMD, which depending
>>>  on the flow rule, may result in anything ranging from empty (single queue)
>>>  to all-inclusive RSS.
>>>
>>> +Best effort will be used, in case there is a conflict inside the rule.
>>> +The conflict can be the result of:
>>> +
>>> +- Conflicting RSS types, for example setting both UDP and TCP.
>>
>> Thinking more about it, I see no conflict at all. It is common
>> to specify both TCP and UDP in hash function if user wants to
>> take TCP ports for TCP packets and UDP ports for UDP into
>> account.
>>
> I fully agree with you that it is common to use both UDP and TCP and
> expect it to work based on the traffic. this is the point of this patch.
> To clarify that this is valid input and the PMD will work in best effort mode.
> 

I think confusion occurs when the the RSS hash function set part of flow
rule rss action.
Than providing a UDP flow and setting UDP & TCP RSS hash types looks
confusing/wrong.

Does mlx support selecting hash function per flow?
Can it be possible to set RSS functions separately from any specific flow?

>
>>> +
>>> +- Conflicting between RSS types and the requested pattern to match,
>>> +  for example matching on UDP and hashing RSS on TCP.
>>
>> I'd not name it a conflict either. It is just common rule when
>> applicable fields are used only.
>>
> Just like my comment from above.
> 
>>> +
>>> +If requested RSS hash type is not supported,
>>> +and no supported hash type is requested then the PMD may reject the flow.
>>> +
>>
>> I disagree with such description. If requested RSS hash type is
>> not supported (not present in dev_info.flow_type_rss_offloads),
>> it must be rejected and error returned.
>> If requested RSS hash type is not supported for a packet
>> matching the rule, but supported in general (present in
>> dev_info.flow_type_rss_offloads), part of RSS hash type
>> specification may be ignored and only applicable bits are used.
>> If result is empty, PMD may reject the flow rule.
>>
> The flow should be rejected even if it is used with some type that is supported?
> 
> 
>>>  Note: RSS hash result is stored in the ``hash.rss`` mbuf field which
>>>  overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi``
>>>  field only, both can be requested simultaneously.
>>>
> 

Re: [dpdk-dev] [PATCH v4] doc: update RSS action with best effort

[Ferruh Yigit]

On 8/10/2020 4:08 PM, Ori Kam wrote:
> Using the rte_flow action RSS types field,
> may result in undefined outcome.
> 
> For example selecting both UDP and TCP,
> selecting TCP RSS type but the pattern is targeting UDP traffic.
> another option is that the PMD doesn't support all requested types.
> 
> Until now, it wasn't clear what will happen in such cases.
> This commit clarify this issue by stating that the PMD
> will work in the best-effort mode, and will fail
> in case the requested type is not supported.
> 
> Signed-off-by: Ori Kam <orika@nvidia.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Applied to dpdk-next-net/main, thanks.