03/10/2018 21:47, Andrew Rybchenko: > On 03.10.2018 21:14, Jerin Jacob wrote: > > From: Andrew Rybchenko <arybche...@solarflare.com> > >>> From: Jerin Jacob <jerin.ja...@caviumnetworks.com> > >>>> From: Andrew Rybchenko <arybche...@solarflare.com> > >>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote: > >>>>> > >>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and > >>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum > >>>>> failure. > >>>>> > >>>>> - To use hardware Rx outer UDP checksum offload, the user needs to > >>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath. > >>>>> > >>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum > >>>>> failure > >>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag. > >>>>> > >>>>> Signed-off-by: Jerin Jacob > >>>>> <jerin.ja...@caviumnetworks.com><mailto:jerin.ja...@caviumnetworks.com> > >>>>> > >>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one > >>>>> patch. > >>>>> It seems typically mbuf changes go separately and mbuf changes > >>>>> should > >>>>> be applied to main dpdk repo. > >>>> I don't have strong opinion on this. If there are no other objection, I > >>>> will split the patch further as mbuf and ethdev as you pointed out. > >>>> > >>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum > >>>>> when > >>>>> 2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM. > >>>>> May be it is OK, but it would be useful to state explicitly why it > >>>>> is decided > >>>>> to go this way. > >>>> I am following the scheme similar to OUTER IP checksum where we have only > >>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit. > >>>> > >>>> > >>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or > >>>>> outer. > >>>>> May be it is not directly related to changeset, but I think it > >>>>> would be really > >>>>> useful to clarify it. > >>>> I will update the comment. > >>> Hi Andrew, > >>> > >>> I looked at the other definitions in mbuf.h, according the documentation, > >>> If nothing is mentioned it is treated as inner if the packet is > >>> tunneled else it is outer most. So I would like avoid confusion by > >>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment. > >>> Technically it is not correct to say "inner" if the packet is not > >>> tunneled. So I am untouching the exiting comment. > >>> > >> Yes, it is incorrect to say that it is inner. How does application find > >> how to treat PKT_RX_L4_CKSUM (inner or outer)? > >> Should it rely on packet type provided in mbuf? > > AFAIK, Finding is it a tunneled packet or not is through ptype or SW has > > to parse the packet. For example, testpmd chooses later method using > > "csum parse-tunnel on <port>" to detect the presence of the tunnel. > > SW parsing of the packet cannot help, since app should be sure > that HW has classified the packet as tunneled and provided information > about inner and outer checksum checks. > > >> Is it specified/mentioned somewhere? > > I don't know. It it not directly related to this change set, Olivier may > > know > > additional details. > > I disagree. You're adding one more offload flag. Yes, it simply follows > existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD > has many open questions. Why should these open questions be preserved > here? It is similar to the code with a bug which is cloned once again with > the bug :) > > If everyone else is fine with the description of Rx checksum offloads and > it is only me who is unhappy with it - no problem.
I agree we must better define these checksum flags. Olivier, please could you give your opinion here?
