| Message ID | 20240418110153.102781-1-aleksandr.mikhalitsyn@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net-next,v2,1/2] ipvs: add READ_ONCE barrier for ipvs->sysctl_amemthresh | expand |
Hello, On Thu, 18 Apr 2024, Alexander Mikhalitsyn wrote: > Cc: Julian Anastasov <ja@ssi.bg> > Cc: Simon Horman <horms@verge.net.au> > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org> > Cc: Florian Westphal <fw@strlen.de> > Suggested-by: Julian Anastasov <ja@ssi.bg> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > --- > net/netfilter/ipvs/ip_vs_ctl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > index 143a341bbc0a..daa62b8b2dd1 100644 > --- a/net/netfilter/ipvs/ip_vs_ctl.c > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > @@ -105,7 +106,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > /* si_swapinfo(&i); */ > /* availmem = availmem - (i.totalswap - i.freeswap); */ > > - nomem = (availmem < ipvs->sysctl_amemthresh); > + amemthresh = max(READ_ONCE(ipvs->sysctl_amemthresh), 0); > + nomem = (availmem < amemthresh); > > local_bh_disable(); > > @@ -146,8 +148,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > case 1: > if (nomem) { > ipvs->drop_rate = ipvs->drop_counter > - = ipvs->sysctl_amemthresh / > - (ipvs->sysctl_amemthresh-availmem); > + = amemthresh / > + (amemthresh-availmem); Thanks, both patches look ok except that the old styling is showing warnings for this patch: scripts/checkpatch.pl --strict /tmp/file1.patch It would be great if you silence them somehow in v3... BTW, est_cpulist is masked with current->cpus_mask of the sysctl writer process, if that is of any help. That is why I skipped it but lets keep it read-only for now... > ipvs->sysctl_drop_packet = 2; > } else { > ipvs->drop_rate = 0; > @@ -156,8 +158,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > case 2: > if (nomem) { > ipvs->drop_rate = ipvs->drop_counter > - = ipvs->sysctl_amemthresh / > - (ipvs->sysctl_amemthresh-availmem); > + = amemthresh / > + (amemthresh-availmem); > } else { > ipvs->drop_rate = 0; > ipvs->sysctl_drop_packet = 1; Regards -- Julian Anastasov <ja@ssi.bg>
On Thu, Apr 18, 2024 at 3:23 PM Julian Anastasov <ja@ssi.bg> wrote: > > > Hello, Dear Julian, > > On Thu, 18 Apr 2024, Alexander Mikhalitsyn wrote: > > > Cc: Julian Anastasov <ja@ssi.bg> > > Cc: Simon Horman <horms@verge.net.au> > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > > Cc: Jozsef Kadlecsik <kadlec@netfilter.org> > > Cc: Florian Westphal <fw@strlen.de> > > Suggested-by: Julian Anastasov <ja@ssi.bg> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > > --- > > net/netfilter/ipvs/ip_vs_ctl.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > > index 143a341bbc0a..daa62b8b2dd1 100644 > > --- a/net/netfilter/ipvs/ip_vs_ctl.c > > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > > > @@ -105,7 +106,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > > /* si_swapinfo(&i); */ > > /* availmem = availmem - (i.totalswap - i.freeswap); */ > > > > - nomem = (availmem < ipvs->sysctl_amemthresh); > > + amemthresh = max(READ_ONCE(ipvs->sysctl_amemthresh), 0); > > + nomem = (availmem < amemthresh); > > > > local_bh_disable(); > > > > @@ -146,8 +148,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > > case 1: > > if (nomem) { > > ipvs->drop_rate = ipvs->drop_counter > > - = ipvs->sysctl_amemthresh / > > - (ipvs->sysctl_amemthresh-availmem); > > + = amemthresh / > > + (amemthresh-availmem); > > Thanks, both patches look ok except that the old styling > is showing warnings for this patch: > > scripts/checkpatch.pl --strict /tmp/file1.patch > > It would be great if you silence them somehow in v3... Yeah, I have fixed this in v3. Also, I had to split multiple assignments into different lines because of: >CHECK: multiple assignments should be avoided Now everything looks fine. > > BTW, est_cpulist is masked with current->cpus_mask of the > sysctl writer process, if that is of any help. That is why I skipped > it but lets keep it read-only for now... That's a good point! Probably I'm too conservative ;-) > > > ipvs->sysctl_drop_packet = 2; > > } else { > > ipvs->drop_rate = 0; > > @@ -156,8 +158,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) > > case 2: > > if (nomem) { > > ipvs->drop_rate = ipvs->drop_counter > > - = ipvs->sysctl_amemthresh / > > - (ipvs->sysctl_amemthresh-availmem); > > + = amemthresh / > > + (amemthresh-availmem); > > } else { > > ipvs->drop_rate = 0; > > ipvs->sysctl_drop_packet = 1; > > Regards Kind regards, Alex > > -- > Julian Anastasov <ja@ssi.bg> >
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 143a341bbc0a..daa62b8b2dd1 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -94,6 +94,7 @@ static void update_defense_level(struct netns_ipvs *ipvs) { struct sysinfo i; int availmem; + int amemthresh; int nomem; int to_change = -1; @@ -105,7 +106,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) /* si_swapinfo(&i); */ /* availmem = availmem - (i.totalswap - i.freeswap); */ - nomem = (availmem < ipvs->sysctl_amemthresh); + amemthresh = max(READ_ONCE(ipvs->sysctl_amemthresh), 0); + nomem = (availmem < amemthresh); local_bh_disable(); @@ -146,8 +148,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) case 1: if (nomem) { ipvs->drop_rate = ipvs->drop_counter - = ipvs->sysctl_amemthresh / - (ipvs->sysctl_amemthresh-availmem); + = amemthresh / + (amemthresh-availmem); ipvs->sysctl_drop_packet = 2; } else { ipvs->drop_rate = 0; @@ -156,8 +158,8 @@ static void update_defense_level(struct netns_ipvs *ipvs) case 2: if (nomem) { ipvs->drop_rate = ipvs->drop_counter - = ipvs->sysctl_amemthresh / - (ipvs->sysctl_amemthresh-availmem); + = amemthresh / + (amemthresh-availmem); } else { ipvs->drop_rate = 0; ipvs->sysctl_drop_packet = 1;
Cc: Julian Anastasov <ja@ssi.bg> Cc: Simon Horman <horms@verge.net.au> Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Jozsef Kadlecsik <kadlec@netfilter.org> Cc: Florian Westphal <fw@strlen.de> Suggested-by: Julian Anastasov <ja@ssi.bg> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> --- net/netfilter/ipvs/ip_vs_ctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)