From: Julian Anastasov <ja@ssi.bg>
To: Dust Li <dust.li@linux.alibaba.com>
Cc: Simon Horman <horms@verge.net.au>,
Pablo Neira Ayuso <pablo@netfilter.org>,
Jiejian Wu <jiejian@linux.alibaba.com>,
netdev@vger.kernel.org, lvs-devel@vger.kernel.org,
linux-kernel <linux-kernel@vger.kernel.org>,
netfilter-devel@vger.kernel.org, coreteam@netfilter.org
Subject: Re: [PATCH v2 net-next] ipvs: make ip_vs_svc_table and ip_vs_svc_fwm_table per netns
Date: Mon, 24 Jul 2023 06:46:22 +0300 (EEST) [thread overview]
Message-ID: <3decf8be-ee64-b3c3-2e6d-4f3f392f6faf@ssi.bg> (raw)
In-Reply-To: <20230724013101.GI6751@linux.alibaba.com>
Hello,
On Mon, 24 Jul 2023, Dust Li wrote:
> On Sun, Jul 23, 2023 at 08:19:54PM +0300, Julian Anastasov wrote:
> >
> > Changes look good to me, thanks! But checkpatch is reporting
> >for some cosmetic changes that you have to do in v3:
> >
> >scripts/checkpatch.pl --strict /tmp/file.patch
>
> Oh, sorry for that! I ignored the CHECKs checkpatch reported, my checkpatch
> shows:
>
>
> $./scripts/checkpatch.pl --strict 0001-ipvs-make-ip_vs_svc_table-and-ip_vs_svc_fwm_table-pe.patch
> CHECK: Prefer using the BIT macro
> #69: FILE: include/net/ip_vs.h:40:
> +#define IP_VS_SVC_TAB_SIZE (1 << IP_VS_SVC_TAB_BITS)
>
> We just moved this line from ip_vs_ctl.c to ip_vs.h, so we ignored the
> BIT macro. Do you think we should change it using BIT macro ?
Yes, lets fix all these complains.
> CHECK: struct mutex definition without comment
> #79: FILE: include/net/ip_vs.h:1051:
> + struct mutex service_mutex;
>
> I think we can add comment for it.
> But rethinking a bit on the service_mutex in ip_vs_est.c, I'm a
> wondering why we are using the service_mutex in estimation ? Is est_mutex
> enough for the protecting in ip_vs_est.c ?
Yes, the estimation kthreads are synchronized only
with reconfiguration: ip_vs_start_estimator() and ip_vs_stop_estimator()
are called under service_mutex. And the estimation data is already
per-net, there is no global data.
> CHECK: Logical continuations should be on the previous line
> #161: FILE: net/netfilter/ipvs/ip_vs_ctl.c:410:
> && (svc->port == vport)
> + && (svc->protocol == protocol)) {
>
> This is just the removal of '(svc->ipvs == ipvs)' and kept it as it is.
> So haven't change according to checkpatch. If you prefer, I can modify
> it to make checkpatch happy.
Yes, lets move all '&&' in this 'if' block and also remove the
parens:
if (svc->af == af && ip_vs_addr_equal(af, &svc->addr, vaddr) &&
svc->port == vport && svc->protocol == protocol) {
Regards
--
Julian Anastasov <ja@ssi.bg>
prev parent reply other threads:[~2023-07-24 3:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-23 15:44 [PATCH v2 net-next] ipvs: make ip_vs_svc_table and ip_vs_svc_fwm_table per netns Dust Li
2023-07-23 17:19 ` Julian Anastasov
2023-07-24 1:31 ` Dust Li
2023-07-24 3:46 ` Julian Anastasov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3decf8be-ee64-b3c3-2e6d-4f3f392f6faf@ssi.bg \
--to=ja@ssi.bg \
--cc=coreteam@netfilter.org \
--cc=dust.li@linux.alibaba.com \
--cc=horms@verge.net.au \
--cc=jiejian@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lvs-devel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).