Message-ID: <20161116190931.GA31421@hmsreliant.think-freely.org>
Date:   Wed, 16 Nov 2016 14:09:31 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Yasevich <vyasevich@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Phil Sutter <phil@....cc>
Subject: Re: [PATCH net] sctp: use new rhlist interface on sctp transport
 rhashtable

On Wed, Nov 16, 2016 at 09:34:52PM +0800, Xin Long wrote:
> On Wed, Nov 16, 2016 at 2:04 AM, Neil Horman <nhorman@...driver.com> wrote:
> > On Tue, Nov 15, 2016 at 11:23:11PM +0800, Xin Long wrote:
> >> Now sctp transport rhashtable uses hash(lport, dport, daddr) as the key
> >> to hash a node to one chain. If in one host thousands of assocs connect
> >> to one server with the same lport and different laddrs (although it's
> >> not a normal case), all the transports would be hashed into the same
> >> chain.
> >>
> >> It may cause to keep returning -EBUSY when inserting a new node, as the
> >> chain is too long and sctp inserts a transport node in a loop, which
> >> could even lead to system hangs there.
> >>
> >> The new rhlist interface works for this case that there are many nodes
> >> with the same key in one chain. It puts them into a list then makes this
> >> list be as a node of the chain.
> >>
> >> This patch is to replace rhashtable_ interface with rhltable_ interface.
> >> Since a chain would not be too long and it would not return -EBUSY with
> >> this fix when inserting a node, the reinsert loop is also removed here.
> >>
> >> Signed-off-by: Xin Long <lucien.xin@...il.com>
> >
> > Does this really buy us anything in this case though?  If the use case is that a
> > majority of the associations map to the same key, then you might avoid EBUSY for
> > the individual associaion that doesn't map there, but you still have to cope
> > with a huge linear search for the majority of the keys.
> >
> 
> This patch is NOT for improving performance, it is to reorganize
> transports in rhashtable in another way to avoid EBUSY, rhlist is born
> for this.
> 
Never said it was a performance issue, just suggested that avoiding EBUSY
returns on inserts might be handled in other ways.

> Before this patch, the transport insert codes are pretty bad, if it returns
> EBUSY, it would retry in a loop. now this patch avoid this and even
> removed that loop, it's a fix for this issue.
> 
> > Might be more reasonable to mix saddr into the hash function so that your use
> > case gets spread through the hash table more evenly.
> 
> we can not do this:
> 1. it will increase rhashtable's size when using multihome, if a host has
>     N addrs, the size for one assoc will be N times bigger than now.
> 2. the hash node is inside transport, if we mix saddrs, when using multihome
>     one transport would be hashed many times with different saddrs, we
>     would have to define new structure to link transport.
> we do not need to do this:
> 1. as the changelog said, "it's not a normal case", in one host (client), it
> shouldn't connect to the same server with different saddrs.
> 2. now as long as paddr+dport+lport are different, rhashtable can hash
> it evenly.

Its the 'not a normal case' thats getting me.  Making a non-trivial change like
this for a corner use case typically makes me suspcious, but your points
regarding multiple hash entries being needed when saddr is used in a multihome
scenario make sense to me.  And looking at the rhltable code more closely, I
think this makes more sense

Acked-by: Neil Horman <nhorman@...driver.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists