Re: [iptables PATCH] nft: Increase BATCH_PAGE_SIZE to support huge rulesets

From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Phil Sutter <phil@nwl.cc>, Florian Westphal <fw@strlen.de>,
	netfilter-devel@vger.kernel.org
Subject: Re: [iptables PATCH] nft: Increase BATCH_PAGE_SIZE to support huge rulesets
Date: Sat, 3 Apr 2021 12:23:36 +0200	[thread overview]
Message-ID: <20210403102336.GA30107@salvia> (raw)
In-Reply-To: <20210403084940.GA3158@orbyte.nwl.cc>

On Sat, Apr 03, 2021 at 10:49:40AM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Apr 02, 2021 at 07:38:10AM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > In order to support the same ruleset sizes as legacy iptables, the
> > > kernel's limit of 1024 iovecs has to be overcome. Therefore increase
> > > each iovec's size from 256KB to 4MB.
> > > 
> > > While being at it, add a log message for failing sendmsg() call. This is
> > > not supposed to happen, even if the transaction fails. Yet if it does,
> > > users are left with only a "line XXX failed" message (with line number
> > > being the COMMIT line).
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  iptables/nft.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/iptables/nft.c b/iptables/nft.c
> > > index bd840e75f83f4..e19c88ece6c2a 100644
> > > --- a/iptables/nft.c
> > > +++ b/iptables/nft.c
> > > @@ -88,11 +88,11 @@ int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
> > >  
> > >  #define NFT_NLMSG_MAXSIZE (UINT16_MAX + getpagesize())
> > >  
> > > -/* selected batch page is 256 Kbytes long to load ruleset of
> > > - * half a million rules without hitting -EMSGSIZE due to large
> > > - * iovec.
> > > +/* Selected batch page is 4 Mbytes long to support loading a ruleset of 3.5M
> > > + * rules matching on source and destination address as well as input and output
> > > + * interfaces. This is what legacy iptables supports.
> > >   */
> > > -#define BATCH_PAGE_SIZE getpagesize() * 32
> > > +#define BATCH_PAGE_SIZE getpagesize() * 512
> > 
> > Why not remove getpagesize() altogether?
> 
> Yes, why not. At least I couldn't find a reason in git log why it's
> there in the first place.
> 
> > The comment assumes getpagesize returns 4096 so might as well just use
> > "#define BATCH_PAGE_SIZE  (4 * 1024 * 1024)" or similar?
> > 
> > On my system getpagesize() * 512 yields 2097152 ...
> 
> Thanks for digging deeper, my comment was wrong. I believed the old
> comment and assumed getpagesize() would return 256k / 32 = 8k but indeed
> it returns 4k.

NLMSG_GOODSIZE is not exposed to userspace, I was using it as
reference. NLMSG_GOODSIZE is PAGE_SIZE.