Hi Ben,

Thanks for the note. Now I could use your proposal to move on.
I think I can't explicitly replace the current code
    hash = hash_add64(hash, *((OVS_FORCE uint64_t *) &flow->dl_dst));
with
    hash = hash_add64(hash, eth_addr_to_uint64(flow->dl_dst));
, since the implementation behind eth_addr_to_uint64() is different,
and as an outcome the codes of miniflow_hash_5tuple() and
flow_hash_5tuple() would differ.

By using eth_addr_to_uint64(), I think the returned value will contain
extra zeroes due to conversion:
static inline uint64_t eth_addr_to_uint64(const struct eth_addr ea)
{
    return (((uint64_t) ntohs(ea.be16[0]) << 32)
            | ((uint64_t) ntohs(ea.be16[1]) << 16)
            | ntohs(ea.be16[2]));
}

, while the other function has non-zero bits from the other field:
        hash = hash_add64(hash, macs.u64.lo);

I believe this could be used if we were only about to get rid of the
warning:
    hash = hash_add64(hash, *((uint64_t *) ((void *) &flow->dl_dst)));
, but my proposal is to replace this 64+32 hash calculation with
2*64 hashes, so we can eliminate this issue safely.

Best Regards,
Gabor

> -----Original Message-----
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: 2018. április 13. 20:00
> To: Gabor Halász <gabor.hal...@ericsson.com>
> Cc: dev <d...@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH v2] flow: Extend 5-tuple hash calculation for
> non-IP packets
> 
> On Thu, Apr 12, 2018 at 01:01:09PM +0000, Gabor Halász wrote:
> > In real-world vSwitch deployments, handling a few thousand flows,
> > EMC is quickly saturated, so it's optimal usage is critical to
> > reach the highest packet forwarding speed of the vSwitch.
> >
> > EMC lookup is initiated based on the hash value of the packet.
> > In case the packet does not already have a stored hash value
> > during processing, the miniflow_hash_5tuple() function is invoked
> > in the datapath. While packets entering the vSwitch from an
> > external interface usually have valid hashes (pre-computed by NICs
> > supporting RSS), the ones coming from vhostuser ports (internal
> > packets from VMs) do not.
> >
> > Non-IP traffic received from the VMs experiences very bad EMC hit
> > rates and hence forwarding performance, because the
> miniflow_hash_5tuple()
> > returns the same hash value and these packets will hit the same EMC
> > entries and cause collisions if there are more than two distinct
> > megaflows with traffic in the PMD.
> >
> > The purpose of the patch is to compute proper hashes with sufficient
> > entropy for EMC lookup also for non-IP traffic to avoid constant EMC
> > thrashing. The hash calculation has been extended to handle unrecognized
> > ethernet types and MPLS, using the header fields that are valid for a
> > specific protocol.
> >
> > Forwarding of non-IP packets in NFVI scenarios is very likely to happen
> > based on MAC addresses and/or VLAN tags.
> > By implementing a special case for matching on MPLS label, this change
> > prepares a separate commit that will enable hash recalculation for MPLS
> > packets received from L3 GRE tunnels. Today we skip re-computation of
> > the hash and the original GRE hash is only updated with the increased
> > recirc_depth.
> >
> > Signed-off-by: Gabor Halasz <gabor.hal...@ericsson.com>
> > ---
> > v1->v2:
> >  - Fix for Clang error "cast increases required alignment from 2 to 4"
> 
> Thanks for v2.  It doesn't make a difference because OVS_FORCE does not
> affect Clang, only sparse.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to