| Message ID | 1620232462-140953-1-git-send-email-danielj@nvidia.com |
|---|---|
| Headers | show |
| Series | Connection tracking fixes | expand |
Acked-by: Tim Gardner <tim.gardner@canonical.com> The end result looks correct, though your SRU explanations are out of order from the patches. On 5/5/21 10:34 AM, 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(-) >
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
> From: Stefan Bader <stefan.bader@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.
On 11.05.21 14:02, Daniel Jurgens wrote: >> From: Stefan Bader <stefan.bader@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