[patch] Source entries removing is awfully slow.

[Kajetan Staszkiewicz]

Dnia poniedziałek, 11 marca 2013 o 16:27:33 Ermal Luçi napisał(a):
> On Mon, Mar 11, 2013 at 4:05 PM, Kajetan Staszkiewicz
> <vegeta at tuxpowered.net
> 
> > wrote:
> > 
> > There are some things I find flawed in your patch:
> > 
> > 1.
> > 
> > +#if 0
> > 
> >                 if (killed > 0)
> >                 
> >                         pf_purge_expired_src_nodes(1);
> > 
> > +#endif
> > 
> > This means that after using `pfctl -K` the src nodes are still around
> > until purged and any new states created will still use them and bump
> > their expire timer. This also changes behavior from DIOCCLRSRCNODES,
> > which also performs the
> > purge immediately. You also moved s->src_node=s->nat_src_node=NULL code
> > to inside of pf_purge_expired_src_nodes, therefore I believe it should
> > be called
> > immediately. If detaching state from source is done in
> > pf_purge_expired_src_nodes, DIOCCLRSRCNODES does not have to traverse the
> > state
> > table anymore, so we achieve another performance improvement.
> 
> Well probably just need to clear the rtaddr member of a source node.

rt_addr is a property of pf_state (or did you mean raddr of pf_src_node?), I do 
not know if the rest of pf code allows it to be just cleared, and even if, it 
would only create broken State entries not forwarding data to the server behind 
the loadbalancer (I *might* want to leave existing connections alive, depending 
on usage of further mentioned pfctl's "-c" switch, but I always want to remove 
Sources ASAP so no new connections use them). And of course I prefer to keep 
the default behavior unchanged.

> Its an optimization to keep the source node there until next purge since
> you would avoid a memory allocation.
> It seems to me a better behaviour rather than having to loop N amount of
> states during an ioctl which is supposed to be fast.

This was the original behavior of the code, I only reduced N to reasonable 
values (and therefore time of operation).

> You need to take into consideration that an ioctl freezes the whole
> operation of network in this case because you are modifying
> the core of pf(4).

Yes, this is why I started working on the issue in the first place. My approach 
introduces no additional overhead, only redurces the existing one.

> I would rather be keen to mark a src_node state as expired and let the
> purge_thread collect that?

What I observed was that without calling pf_purge_expired_src_nodes (your 
patch), Sources were still displayed with 0 expiration time and if any client 
connected before they got purged, expiration time was bumped up and the Source 
entry was alive again and never purged anymore. This is definetely not the 
behavior I'd expect.

> 
> > 2.
> > 
> >                 /* Handle state to src_node linkage */
> > 
> > +#ifndef __FreeBSD__
> > 
> >                 if (sn->states != 0) {
> >                 
> >                     RB_FOREACH(s, pf_state_tree_id,
> > 
> > #ifdef __FreeBSD__
> > 
> >                         &V_tree_id) {
> > 
> > #else
> > 
> >                         &tree_id) {
> > 
> > #endif
> > 
> >                         if (s->src_node == sn)
> >                         
> >                             s->src_node = NULL;
> >                         
> >                         if (s->nat_src_node == sn)
> >                         
> >                             s->nat_src_node = NULL;
> >                     
> >                     }
> >                     sn->states = 0;
> >                 
> >                 }
> > 
> > +#endif
> > 
> >                 sn->expire = 1;
> >                 killed++;
> > 
> > This removes a bit too much code, that is zeroing of source's state
> > counter.
> > 
> > Please find the next version of the patch here:
> > http://vegeta.tuxpowered.net/download/link-states-to-src_node-3.patch
> > 
> > This one also takes care of removing states linked to found sources if
> > pfctl is
> > given extra -c parameter (that can stand for "clear", I could not find
> > any other free pfctl parameter better matching). Thanks to this
> > parameter, the default behavior is not changed.
> 
> Its ok for the extra -c, there is even -S free just to be consistent with
> the flush and vieweing options probably better use that?!

> The addition of 2 extra members to pf_state structure i am reluctant a bit
> since its just for reporting the number of states killed
> and it really is not a useful information compared to the increased size of
> the structure that consumes RAM memory.

Variable for counting terminated states was added to pfioc_src_node_kill, not 
to pf_state.

> Also pf_src_node structure if the 2 options for display are not added can
> use the member that is used to report
> the number of states killed to request killing linked states.
> What do you think?

Again, the counter is in pfioc_src_node_kill, not pf_src_node.

-- 
| pozdrawiam / greetings | powered by Debian, CentOS and FreeBSD |
|  Kajetan Staszkiewicz  | jabber,email: vegeta()tuxpowered net  |
|        Vegeta          | www: http://vegeta.tuxpowered.net     |
`------------------------^---------------------------------------'