Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Julian Anastasov <ja@ssi.bg>
To: Simon Horman <horms@verge.net.au>
Cc: lvs-devel@vger.kernel.org,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	netfilter-devel@vger.kernel.org,
	Andrew Sy Kim <kim.andrewsy@gmail.com>
Subject: Re: [PATCH net-next] ipvs: avoid expiring many connections from timer
Date: Tue, 30 Jun 2020 19:10:06 +0300 (EEST)
Message-ID: <alpine.LFD.2.22.394.2006301851400.4282@ja.home.ssi.bg> (raw)
In-Reply-To: <20200630151922.GA12560@vergenet.net>


	Hello,

On Tue, 30 Jun 2020, Simon Horman wrote:

> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> > index 02f2f636798d..b3921ae92740 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c

> > @@ -827,14 +852,17 @@ static void ip_vs_conn_expire(struct timer_list *t)
> >  
> >  		/* does anybody control me? */
> >  		if (ct) {
> > +			bool has_ref = !cp->timeout && __ip_vs_conn_get(ct);
> > +
> >  			ip_vs_control_del(cp);
> >  			/* Drop CTL or non-assured TPL if not used anymore */
> > -			if (!cp->timeout && !atomic_read(&ct->n_control) &&
> > +			if (has_ref && !atomic_read(&ct->n_control) &&
> >  			    (!(ct->flags & IP_VS_CONN_F_TEMPLATE) ||
> >  			     !(ct->state & IP_VS_CTPL_S_ASSURED))) {
> >  				IP_VS_DBG(4, "drop controlling connection\n");
> > -				ct->timeout = 0;
> > -				ip_vs_conn_expire_now(ct);
> > +				ip_vs_conn_del_put(ct);
> 
> Previously this code did not put the ct, now it does.
> Is that intentional.

	Yes, as ip_vs_conn_expire() now can be called both in
timer and process context we need a reference for ct while
calling del_timer() in ip_vs_conn_del_put(). As ct->n_control
is 0 after our ip_vs_control_del(), ct can be expired by
timer while we are trying to del it here.

> > @@ -1341,19 +1368,15 @@ static void ip_vs_conn_flush(struct netns_ipvs *ipvs)
> >  		hlist_for_each_entry_rcu(cp, &ip_vs_conn_tab[idx], c_list) {
> >  			if (cp->ipvs != ipvs)
> >  				continue;
> > -			/* As timers are expired in LIFO order, restart
> > -			 * the timer of controlling connection first, so
> > -			 * that it is expired after us.
> > -			 */
> > +			if (atomic_read(&cp->n_control))
> > +				continue;
> >  			cp_c = cp->control;
> > -			/* cp->control is valid only with reference to cp */
> > -			if (cp_c && __ip_vs_conn_get(cp)) {
> > +			IP_VS_DBG(4, "del connection\n");
> > +			ip_vs_conn_del(cp);
> > +			if (cp_c && !atomic_read(&cp_c->n_control)) {
> >  				IP_VS_DBG(4, "del controlling connection\n");
> > -				ip_vs_conn_expire_now(cp_c);
> > -				__ip_vs_conn_put(cp);
> > +				ip_vs_conn_del(cp_c);
> 
> Conversely, previously this code put the ct, now it doesn't.
> Is that also intentional?

	Now we do not get reference to cp because in RCU
section the cp structure can not go away. So, we have an
implicit reference to cp. Same for its cp->control (ct).
The conn structures are freed later in RCU callback.

	In this case we may run del_timer() after
another CPU, eg. after ip_vs_conn_expire() was already
called after timer expire or after ip_vs_conn_del*(). But
for us del_timer will not succeed.

Regards

--
Julian Anastasov <ja@ssi.bg>

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-20 10:03 Julian Anastasov
2020-06-30 15:19 ` Simon Horman
2020-06-30 16:10   ` Julian Anastasov [this message]
2020-07-01  6:54     ` Simon Horman
2020-07-01  8:17 ` Pablo Neira Ayuso

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=alpine.LFD.2.22.394.2006301851400.4282@ja.home.ssi.bg \
    --to=ja@ssi.bg \
    --cc=horms@verge.net.au \
    --cc=kim.andrewsy@gmail.com \
    --cc=lvs-devel@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

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git