mbox series

[0/7] net/mlx5: fixes for the new flow engine

Message ID 20181008180150.39273-1-yskoh@mellanox.com (mailing list archive)
Headers
Series net/mlx5: fixes for the new flow engine |

Message

Yongseok Koh Oct. 8, 2018, 6:02 p.m. UTC 
  Minor fixes accumulated since the following two patchsets.

	net/mlx5: add Direct Verbs flow driver support [1]
	net/mlx5: migrate Linux TC flower driver to new flow engine

[1] http://patches.dpdk.org/cover/45248
[2] http://patches.dpdk.org/cover/44897

Yongseok Koh (7):
  net/mlx5: fix wrong flow action macro usage
  net/mlx5: use standard IP protocol numbers
  net/mlx5: rename flow macros
  net/mlx5: fix validation of VLAN ID in flow spec
  net/mlx5: fix flow validation for no fate action
  net/mlx5: add missing VLAN action constraints
  net/mlx5: fix errno values for flow engine

 drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++------------------
 drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
 drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
 drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
 drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
 5 files changed, 193 insertions(+), 145 deletions(-)

Comments

Shahaf Shuler Oct. 9, 2018, 8:58 a.m. UTC | #1 
Monday, October 8, 2018 9:02 PM, Yongseok Koh:
> Subject: [PATCH 0/7] net/mlx5: fixes for the new flow engine
> 
> Minor fixes accumulated since the following two patchsets.
> 
> 	net/mlx5: add Direct Verbs flow driver support [1]
> 	net/mlx5: migrate Linux TC flower driver to new flow engine
> 
> [1] http://patches.dpdk.org/cover/45248
> [2] http://patches.dpdk.org/cover/44897
> 
> Yongseok Koh (7):
>   net/mlx5: fix wrong flow action macro usage
>   net/mlx5: use standard IP protocol numbers
>   net/mlx5: rename flow macros
>   net/mlx5: fix validation of VLAN ID in flow spec
>   net/mlx5: fix flow validation for no fate action
>   net/mlx5: add missing VLAN action constraints
>   net/mlx5: fix errno values for flow engine
> 
>  drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++---------------
> ---
>  drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
>  drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
>  drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
>  drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
>  5 files changed, 193 insertions(+), 145 deletions(-)

Series applied to next-net-mlx, thanks. 
Need to add the missing VLAN limitation of the "pop always to non-uplink" on a separate commit, don't want to stall the entire series. 

> 
> --
> 2.11.0
Ferruh Yigit Oct. 9, 2018, 3:49 p.m. UTC | #2 
On 10/9/2018 9:58 AM, Shahaf Shuler wrote:
> Monday, October 8, 2018 9:02 PM, Yongseok Koh:
>> Subject: [PATCH 0/7] net/mlx5: fixes for the new flow engine
>>
>> Minor fixes accumulated since the following two patchsets.
>>
>> 	net/mlx5: add Direct Verbs flow driver support [1]
>> 	net/mlx5: migrate Linux TC flower driver to new flow engine
>>
>> [1] http://patches.dpdk.org/cover/45248
>> [2] http://patches.dpdk.org/cover/44897
>>
>> Yongseok Koh (7):
>>   net/mlx5: fix wrong flow action macro usage
>>   net/mlx5: use standard IP protocol numbers
>>   net/mlx5: rename flow macros
>>   net/mlx5: fix validation of VLAN ID in flow spec
>>   net/mlx5: fix flow validation for no fate action
>>   net/mlx5: add missing VLAN action constraints
>>   net/mlx5: fix errno values for flow engine
>>
>>  drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++---------------
>> ---
>>  drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
>>  drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
>>  drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
>>  drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
>>  5 files changed, 193 insertions(+), 145 deletions(-)
> 
> Series applied to next-net-mlx, thanks. 
> Need to add the missing VLAN limitation of the "pop always to non-uplink" on a separate commit, don't want to stall the entire series. 

Hi Shahaf,

These are fixing mlx5 patches which are merged very recently, that is why I
tried to squash these to original commit. This is both for more clean git
history and to have correct Fixes information in commit logs.

I can able to squash: 1/7, 2/7 & 4/7

But 4 are still remaining, main reason is they fixes more than one commit so not
easy to squash into one.

I won't merge remaining ones for now, can you please rebase them on top of
next-net, and try to arrange in a way to squash into next-net, if not able to
make we can get them as it is.
Shahaf Shuler Oct. 10, 2018, 5:57 a.m. UTC | #3 
Tuesday, October 9, 2018 6:49 PM, Ferruh Yigit:
> Subject: Re: [dpdk-dev] [PATCH 0/7] net/mlx5: fixes for the new flow engine
> 
> On 10/9/2018 9:58 AM, Shahaf Shuler wrote:
> > Monday, October 8, 2018 9:02 PM, Yongseok Koh:
> >> Subject: [PATCH 0/7] net/mlx5: fixes for the new flow engine

[...]

> >> djWRGvBzaqpfUVsn8%3D&reserved=0
> >>
> >> Yongseok Koh (7):
> >>   net/mlx5: fix wrong flow action macro usage
> >>   net/mlx5: use standard IP protocol numbers
> >>   net/mlx5: rename flow macros
> >>   net/mlx5: fix validation of VLAN ID in flow spec
> >>   net/mlx5: fix flow validation for no fate action
> >>   net/mlx5: add missing VLAN action constraints
> >>   net/mlx5: fix errno values for flow engine
> >>
> >>  drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++-----------
> ----
> >> ---
> >>  drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
> >>  drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
> >>  drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
> >>  drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
> >>  5 files changed, 193 insertions(+), 145 deletions(-)
> >
> > Series applied to next-net-mlx, thanks.
> > Need to add the missing VLAN limitation of the "pop always to non-uplink"
> on a separate commit, don't want to stall the entire series.
> 
> Hi Shahaf,
> 
> These are fixing mlx5 patches which are merged very recently, that is why I
> tried to squash these to original commit. This is both for more clean git
> history and to have correct Fixes information in commit logs.

I am not sure I agree here. There was a feature which was accepted and later on it had some bug fixes. 
I think it is better to separate between the two, because:
1. I don't think it spams that much the git history
2. if the small fix raised a different bug it will be easier to bisect and track it rather than trying to check what is wrong on the big feature patch

> 
> I can able to squash: 1/7, 2/7 & 4/7
> 
> But 4 are still remaining, main reason is they fixes more than one commit so
> not easy to squash into one.
> 
> I won't merge remaining ones for now, can you please rebase them on top of
> next-net, and try to arrange in a way to squash into next-net, if not able to
> make we can get them as it is.

If it is not critical for you, I suggest we take them as is. It will require some work to re-arrange them + test again.
Let me know.
Ferruh Yigit Oct. 10, 2018, 10:57 a.m. UTC | #4 
On 10/10/2018 6:57 AM, Shahaf Shuler wrote:
> Tuesday, October 9, 2018 6:49 PM, Ferruh Yigit:
>> Subject: Re: [dpdk-dev] [PATCH 0/7] net/mlx5: fixes for the new flow engine
>>
>> On 10/9/2018 9:58 AM, Shahaf Shuler wrote:
>>> Monday, October 8, 2018 9:02 PM, Yongseok Koh:
>>>> Subject: [PATCH 0/7] net/mlx5: fixes for the new flow engine
> 
> [...]
> 
>>>> djWRGvBzaqpfUVsn8%3D&reserved=0
>>>>
>>>> Yongseok Koh (7):
>>>>   net/mlx5: fix wrong flow action macro usage
>>>>   net/mlx5: use standard IP protocol numbers
>>>>   net/mlx5: rename flow macros
>>>>   net/mlx5: fix validation of VLAN ID in flow spec
>>>>   net/mlx5: fix flow validation for no fate action
>>>>   net/mlx5: add missing VLAN action constraints
>>>>   net/mlx5: fix errno values for flow engine
>>>>
>>>>  drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++-----------
>> ----
>>>> ---
>>>>  drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
>>>>  drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
>>>>  drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
>>>>  drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
>>>>  5 files changed, 193 insertions(+), 145 deletions(-)
>>>
>>> Series applied to next-net-mlx, thanks.
>>> Need to add the missing VLAN limitation of the "pop always to non-uplink"
>> on a separate commit, don't want to stall the entire series.
>>
>> Hi Shahaf,
>>
>> These are fixing mlx5 patches which are merged very recently, that is why I
>> tried to squash these to original commit. This is both for more clean git
>> history and to have correct Fixes information in commit logs.
> 
> I am not sure I agree here. There was a feature which was accepted and later on it had some bug fixes. 
> I think it is better to separate between the two, because:
> 1. I don't think it spams that much the git history
> 2. if the small fix raised a different bug it will be easier to bisect and track it rather than trying to check what is wrong on the big feature patch

I would agree but for this case original feature is too fresh. I think it is
opportunity to merge it into original feature. Eventually final code will be
same, if there is a bug it will be in both ways, just to improve the history.

And I didn't understand why it is better to "fix the fix" instead of merge the
fix to the original feature and "fix the feature" if the fix is wrong.

Also this "fix the fix" chains may make reading/understanding original code
harder in the future.

> 
>>
>> I can able to squash: 1/7, 2/7 & 4/7
>>
>> But 4 are still remaining, main reason is they fixes more than one commit so
>> not easy to squash into one.
>>
>> I won't merge remaining ones for now, can you please rebase them on top of
>> next-net, and try to arrange in a way to squash into next-net, if not able to
>> make we can get them as it is.
> 
> If it is not critical for you, I suggest we take them as is. It will require some work to re-arrange them + test again.
> Let me know. 

No it is not critical, but again patchset doesn't look like too big, so if it is
not too much effort I prefer squashing them. And as a final result code should
be exact same, so testing effort shouldn't change but re-arrange takes effort, I
already did a few of them for you...
Ferruh Yigit Oct. 10, 2018, 1:01 p.m. UTC | #5 
On 10/10/2018 11:57 AM, Ferruh Yigit wrote:
> On 10/10/2018 6:57 AM, Shahaf Shuler wrote:
>> Tuesday, October 9, 2018 6:49 PM, Ferruh Yigit:
>>> Subject: Re: [dpdk-dev] [PATCH 0/7] net/mlx5: fixes for the new flow engine
>>>
>>> On 10/9/2018 9:58 AM, Shahaf Shuler wrote:
>>>> Monday, October 8, 2018 9:02 PM, Yongseok Koh:
>>>>> Subject: [PATCH 0/7] net/mlx5: fixes for the new flow engine
>>
>> [...]
>>
>>>>> djWRGvBzaqpfUVsn8%3D&reserved=0
>>>>>
>>>>> Yongseok Koh (7):
>>>>>   net/mlx5: fix wrong flow action macro usage
>>>>>   net/mlx5: use standard IP protocol numbers
>>>>>   net/mlx5: rename flow macros
>>>>>   net/mlx5: fix validation of VLAN ID in flow spec
>>>>>   net/mlx5: fix flow validation for no fate action
>>>>>   net/mlx5: add missing VLAN action constraints
>>>>>   net/mlx5: fix errno values for flow engine
>>>>>
>>>>>  drivers/net/mlx5/mlx5_flow.c       | 117 +++++++++++++++++++-----------
>>> ----
>>>>> ---
>>>>>  drivers/net/mlx5/mlx5_flow.h       |  54 ++++++++---------
>>>>>  drivers/net/mlx5/mlx5_flow_dv.c    |  30 +++++-----
>>>>>  drivers/net/mlx5/mlx5_flow_tcf.c   |  78 +++++++++++++++++++------
>>>>>  drivers/net/mlx5/mlx5_flow_verbs.c |  59 ++++++++++---------
>>>>>  5 files changed, 193 insertions(+), 145 deletions(-)
>>>>
>>>> Series applied to next-net-mlx, thanks.
>>>> Need to add the missing VLAN limitation of the "pop always to non-uplink"
>>> on a separate commit, don't want to stall the entire series.
>>>
>>> Hi Shahaf,
>>>
>>> These are fixing mlx5 patches which are merged very recently, that is why I
>>> tried to squash these to original commit. This is both for more clean git
>>> history and to have correct Fixes information in commit logs.
>>
>> I am not sure I agree here. There was a feature which was accepted and later on it had some bug fixes. 
>> I think it is better to separate between the two, because:
>> 1. I don't think it spams that much the git history
>> 2. if the small fix raised a different bug it will be easier to bisect and track it rather than trying to check what is wrong on the big feature patch
> 
> I would agree but for this case original feature is too fresh. I think it is
> opportunity to merge it into original feature. Eventually final code will be
> same, if there is a bug it will be in both ways, just to improve the history.
> 
> And I didn't understand why it is better to "fix the fix" instead of merge the
> fix to the original feature and "fix the feature" if the fix is wrong.
> 
> Also this "fix the fix" chains may make reading/understanding original code
> harder in the future.
> 
>>
>>>
>>> I can able to squash: 1/7, 2/7 & 4/7
>>>
>>> But 4 are still remaining, main reason is they fixes more than one commit so
>>> not easy to squash into one.
>>>
>>> I won't merge remaining ones for now, can you please rebase them on top of
>>> next-net, and try to arrange in a way to squash into next-net, if not able to
>>> make we can get them as it is.
>>
>> If it is not critical for you, I suggest we take them as is. It will require some work to re-arrange them + test again.
>> Let me know. 
> 
> No it is not critical, but again patchset doesn't look like too big, so if it is
> not too much effort I prefer squashing them. And as a final result code should
> be exact same, so testing effort shouldn't change but re-arrange takes effort, I
> already did a few of them for you...

All squashed except from 6/7 & "7/7 partially", I can confirm final code has not
changed but please double check latest next-net head.