Re: [dpdk-dev] [PATCH v2 1/2] librte_net: add crc init and compute APIs

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2017-03-15 19:03, Dumitrescu, Cristian:
> ... <snip>
> 
> > > > > > I think it should be in librte_hash.
> > > > > >
> > > > > > Please check lib/librte_hash/rte_hash_crc.h
> > > > >
> > > > > Is it good to include payload crc calculation in hash library as I see all
> > hash
> > > > related functionality there?
> > > >
> > > > I think yes. Pablo?
> > >
> > > I think this doesn't belong in the hash library. These new functions calculate
> > CRC, but not as a hash function.
> > 
> > Can't we say that a CRC is a hash? What is a hash?
> > A function generating the same output bytes from given input bytes.
> > 
> > I think you must separate hash functions and hash table management.
> > 
> 
> The fact that CRC32 instruction is opportunistically used to compute a hash digest/signature for load balancing (affinity-based) or hash tables (flow tables, ARP cache, etc) does not mean that all the code that uses CRC32 instruction should be placed in librte_hash.
> 
> The purpose of the hash functions in librte_hash is to compute a digest/signature for a given array of bytes (the key) as fast as possible. Any hash function that produces a hash signature with good uniform distribution in a small amount of cycles belongs here, including those opportunistically using specialized CPU instructions such as CRC32 (or XOR, AESNI, etc).
> 
> The code proposed in this patch is used to compute packet fields for various protocols that have a CRC field, such as FCS of Ethernet frames, etc. according to the relevant standard (IEEE 802, others). It is an utility to be used for implementing protocol-level functionality for various protocols from the network stack, similar to e.g. IP or UDP checksum. If we were to add an IP or UDCP checksum calculator, would you put it in librte_hash?
> 
> The code from this patch is never going to be used to compute a fast signature/digest. Typically this CRC is computed over the entire frame/packet rather than just selected fields from the packet representing the application-specific flow key. Also note that the signature produced by CRC32 hash function from librte_hash is actually not the correct Cyclic Redundancy Check of that array of bytes (or, for math guys, of the associated polynomial), it is just a partial/intermediate value.
> 
> Therefore, I suggest placing this code into: librte_ether (given that it can be used to compute Ethernet FCS), or librte_net, or librte_crc. Definitely not in librte_hash.

I agree with you Cristian that the protocol layer must be in librte_net.
But I think most of this patch is not protocol level.
I think you agree with me that the code computing a
"digest/signature for a given array of bytes" must go to librte_hash?