Re: [dpdk-dev] [PATCH 0/7] net/mlx5: fixes for the new flow engine

[Ferruh Yigit <ferruh.yigit@intel.com>]

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