NACK/Cmnt: [SRU][F:linux-bluefield][PATCH 0/4] Connection tracking fixes

[Stefan Bader]

On 11.05.21 14:02, Daniel Jurgens wrote:
>> From: Stefan Bader <stefan.bader at canonical.com>
>> Sent: Tuesday, May 11, 2021 2:48 AM
>> On 05.05.21 18:34, Daniel Jurgens wrote:
>>> These patches aren't neccsarily related, but are order dependent, so
>>> sending as a series. SRU justification for each are provided below.
>>>
>>> Alaa Hleihel (2):
>>>     net/sched: act_ct: Make tcf_ct_flow_table_restore_skb inline
>>>     netfilter: flowtable: Make nf_flow_table_offload_add/del_cb inline
>>>
>>> SRU Justification:
>>>
>>> The patches are for removing module dependency between software
>> module and driver module.
>>>
>>> * Explain the bug(s)
>>>
>>> Without the patches there is a dependency between mlx5_core, act_ct
>> and nf_flow_table.
>>>
>>> * brief explanation of fixes
>>>
>>> The fix is moving used function from c source file to header file.
>>>
>>> * How to test
>>>
>>> Check module dependency with modinfo, lsmod, etc.
>>>
>>> * What it could break.
>>>
>>> If a sofwtare module doesn't exists, i.e. disabling in .config. then
>>> the driver module, mlx5_core, in this case will fail to load.
>>>
>>>
>>>
>>> Paul Blakey (1):
>>>     netfilter: flowtable: Use rw sem as flow block lock
>>>
>>> SRU Justification:
>>>
>>> Increase flow insertion rate by using rw lock instead of mutex on the flow
>> block.
>>>
>>> * Explain the bug(s)
>>>
>>> Insertion rate is done in multiple threads but there is a mutex lock
>>> between looking for the flow block which slows things down. So use a rw
>> lock and take a read lock which is sufficient.
>>>
>>> * brief explanation of fixes
>>>
>>> Use a rw lock on the flow block.
>>>
>>> * How to test
>>>
>>> CT offload and check insertion rate of 5 tuples rules.
>>>
>>> * What it could break.
>>>
>>> Flow insertion rate.
>>>
>>>
>>>
>>> Roi Dayan (1):
>>>     netfilter: flowtable: Free block_cb when being deleted
>>>
>>> SRU Justification:
>>>
>>> Free block_cb memory when asked to be deleted
>>>
>>> * Explain the bug(s)
>>>
>>> Missing memory cleanup.
>>>
>>> * brief explanation of fixes
>>>
>>> Free the memory
>>>
>>> * How to test
>>>
>>> Can be tested with kmemleak. Do ct offload then stop and clean tc ct rules
>> and scan kmemleak.
>>>
>>> * What it could break.
>>>
>>> Memleak.
>>>
>>>    include/net/netfilter/nf_flow_table.h | 51
>> +++++++++++++++++++++++++++++++----
>>>    include/net/tc_act/tc_ct.h            | 11 +++++++-
>>>    net/netfilter/nf_flow_table_core.c    | 46 +------------------------------
>>>    net/netfilter/nf_flow_table_offload.c |  4 +--
>>>    net/sched/act_ct.c                    | 11 --------
>>>    5 files changed, 59 insertions(+), 64 deletions(-)
>>>
>> This mixes multiple bug reports in one submission. If the patches are
>> required to be applied together, this should be one bug report. If things are
>> independent the multiple bug reports would be correct but then each of the
>> patches which belong to the same bug report should be submitted
>> individually. Then those can be looked at and applied without relying on each
>> other.
>>
>> -Stefan
> 
> Hi Stefan, I thought it would make your lives easier to send them as a series, because if they aren't applied in this order there is a merge conflict, because the rw_sem patch touches the same spot as 2 of the others. I can break them up if you prefer.
> 
Hm, you mean though the issues are not related there is a relation between the 
patches. This is a tough call, I guess. I would still tend to have issues / bug 
reports sent individually in case there is some argument on one and not on the 
other ones. In this case I assume there will be context mismatch which could be 
noted in the cover email. Generally the smaller units are the simpler to get 
through the process.

-Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210511/eebeb919/attachment.sig>