Flow ID, LACP, and igb

[Andre Oppermann]

On 29.08.2013 01:42, Alan Somers wrote:
> On Mon, Aug 26, 2013 at 2:40 PM, Andre Oppermann <andre at freebsd.org> wrote:
>
>> On 26.08.2013 19:18, Justin T. Gibbs wrote:
>>
>>> Hi Net,
>>>
>>> I'm an infrequent traveler through the networking code and would
>>> appreciate some feedback on some proposed solutions to issues Spectra
>>> has seen with outbound LACP traffic.
>>>
>>> lacp_select_tx_port() uses the flow ID if it is available in the outbound
>>> mbuf to select the outbound port.  The igb driver uses the msix queue of
>>> the inbound packet to set a packet's flow ID.  This doesn't provide enough
>>> bits of information to yield a high quality flow ID.  If, for example, the
>>> switch controlling inbound packet distribution does a poor job, the
>>> outbound
>>> packet distribution will also be poorly distributed.
>>>
>>
>> Please note that inbound and outbound flow ID do not need to be the same
>> or symmetric.  It only should stay the same for all packets in a single
>> connection to prevent reordering.
>>
>> Generally it doesn't matter if in- and outbound packets do not use the
>> same queue.  Only in sophisticated setups with full affinity, which we
>> don't support yet, it could matter.
>>
>>
>>   The majority of the adapters supported by this driver will compute
>>> the Toeplitz RSS hash.  Using this data seems to work quite well
>>> in our tests (3 member LAGG group).  Is there any reason we shouldn't
>>> use the RSS hash for flow ID?
>>>
>>
>> Using the RSS hash is the idea.  The infrastructure and driver adjustments
>> haven't been implemented throughout yet.
>>
>>
>>   We also tried disabling the use of flow ID and doing the hash directly in
>>> the driver.  Unfortunately, the current hash is pretty weak.  It
>>> multiplies
>>> by 33, which yield very poor distributions if you need to mod the result
>>> by 3 (e.g. LAGG group with 3 members).  Alan modified the driver to use
>>> the FNV hash, which is already in the kernel, and this yielded much better
>>> results.  He is still benchmarking the impact of this change.  Assuming we
>>> can get decent flow ID data, this should only impact outbound UDP, since
>>> the
>>> stack doesn't provide a flow ID in this case.
>>>
>>> Are there other checksums we should be looking at in addition to FNV?
>>>
>>
>> siphash24() is fast, keyed and strong.
>>
> I benchmarked hash32 (the existing hash function) vs fnv_hash using both
> TCP and UDP, with 1500 and 9000 byte MTUs.  At 10Gbps, I couldn't measure
> any difference in either throughput or cpu utilization.  Given that
> siphash24 is definitely slower than hash32, there's no way that I'll find
> it to be significantly faster than fnv_hash for this application.  In fact,
> I'm guessing that it will be slower due to the function call overhead and
> the fact that lagg_hashmbuf calls the hash function on very short buffers.

No problem with fnv_hash().  While I agree that it is likely that siphash24()
is slower if you could afford the time do a test run it would be great to from
guess to know.

> Therefore I'm going to commit the change using fnv_hash in the next few
> days if no one objects.  Here's the diff:
>
> ==== //SpectraBSD/stable/sys/net/ieee8023ad_lacp.c#4 (text) ====
>
> @@ -763,7 +763,6 @@
>       sc->sc_psc = (caddr_t)lsc;
>       lsc->lsc_softc = sc;
>
> -    lsc->lsc_hashkey = arc4random();
>       lsc->lsc_active_aggregator = NULL;
>       LACP_LOCK_INIT(lsc);
>       TAILQ_INIT(&lsc->lsc_aggregators);
> @@ -841,7 +840,7 @@
>       if (sc->use_flowid && (m->m_flags & M_FLOWID))
>           hash = m->m_pkthdr.flowid;
>       else
> -        hash = lagg_hashmbuf(sc, m, lsc->lsc_hashkey);
> +        hash = lagg_hashmbuf(sc, m);
>       hash %= pm->pm_count;
>       lp = pm->pm_map[hash];

The reason for the hashkey was to prevent directed "attacks" on the load
balancing by choosing/predicting the outcome of it.  This is good and bad
as it is undeterministic between runs, which makes debugging particular
situations harder.  To work around the lack of key for fnv_hash() XOR'ing
the hash output with a pre-initialized random is likely sufficient.
The true importance of this randomization is debatable and just point out
why it was there, not to object to you removing it.

-- 
Andre