GRO lib requires users to provide correct mbuf->l2_len/packet_type etc.. This is for avoiding parsing packet headers. If we believe users give correct packet_type, we also should believe l2_len/l4_len etc. are correct. Otherwise, we need to get layer 4 header length from ipv4 header, rather than mbuf->l4_len.
rte_gro_rx_prepare() is a good way to do sanity check or prepare l2_len etc. fields, but this function should be optional, IMO. Either user or rte_gro_rx_prepare() provides correct l2_len etc. value. Thanks, Jiayu From: yang_y_yi <yang_y...@163.com> Sent: Wednesday, September 2, 2020 1:58 PM To: Hu, Jiayu <jiayu...@intel.com> Cc: dev@dpdk.org; tho...@monjalon.net; yangy...@inspur.com Subject: Re:RE: Re:Re: [dpdk-dev] [PATCH] gro: add UDP GRO and VXLAN UDP GRO support Importance: High Jiayu, TCP header length is variable if there is TCP option, it is usually 20 if no TCP option, but correct value must be between 20 and 60 (including 20 and 60), I think we can't assume l4 header length has been set correctly if packet_type is set correctly, this is my point. I think it will be better if we can add a rte_gro_rx_prepare as rte_eth_tx_prepare. GRO can't work normally only if one of all the related fields isn't set correctly. So I don't think such sanity check is a bad thing. At 2020-09-02 13:44:56, "Hu, Jiayu" <jiayu...@intel.com<mailto:jiayu...@intel.com>> wrote: Yi, Packet type is checked by mbuf->packet_type field, and input packets should provide correct packet_type value. TCP GRO only process TCP packets whose header length is between 20 and 60 bytes. So we check l4_len. From: yang_y_yi <yang_y...@163.com<mailto:yang_y...@163.com>> Sent: Tuesday, September 1, 2020 4:43 PM To: Hu, Jiayu <jiayu...@intel.com<mailto:jiayu...@intel.com>> Cc: dev@dpdk.org<mailto:dev@dpdk.org>; tho...@monjalon.net<mailto:tho...@monjalon.net>; yangy...@inspur.com<mailto:yangy...@inspur.com> Subject: Re:Re: [dpdk-dev] [PATCH] gro: add UDP GRO and VXLAN UDP GRO support Jiayu, BTW, after I check it again, I think udp header length check is necessary, it is actually a sanity check io order to ensure it is indeed a udp packet, gro_tcp4.c did same thing. At 2020-09-01 14:10:41, "yang_y_yi" <yang_y...@163.com<mailto:yang_y...@163.com>> wrote: >At 2020-09-01 12:27:29, "Hu, Jiayu" ><jiayu...@intel.com<mailto:jiayu...@intel.com>> wrote: >>Hi Yi, >> >>This patch supports UDP and VxLAN/UDP, but both are in one patch. >>It's too large, and please split it into small patches. > >Jiayu, thank you so much for your great review , I'll send v2 to split it into >two patches and fix your comments. Detailed replies for comments embedded, >please check them. > >> >>Thanks, >>Jiayu >> >>> -----Original Message----- >>> From: yang_y...@163.com<mailto:yang_y...@163.com> >>> <yang_y...@163.com<mailto:yang_y...@163.com>> >>> Sent: Wednesday, July 1, 2020 2:48 PM >>> To: dev@dpdk.org<mailto:dev@dpdk.org> >>> Cc: Hu, Jiayu <jiayu...@intel.com<mailto:jiayu...@intel.com>>; >>> tho...@monjalon.net<mailto:tho...@monjalon.net>; >>> yangy...@inspur.com<mailto:yangy...@inspur.com>; >>> yang_y...@163.com<mailto:yang_y...@163.com> >>> Subject: [PATCH] gro: add UDP GRO and VXLAN UDP GRO support >>> >>> From: Yi Yang <yangy...@inspur.com<mailto:yangy...@inspur.com>> >>> >>> UDP GRO and VXLAN UDP GRO can help improve VM-to-VM >>> UDP performance when VM is enabled UFO or GSO, GRO >>> must be supported if GSO, UFO or VXLAN UFO is enabled >>> , otherwise, performance gain will be hurt. >>> >>> With this enabled in DPDK, OVS DPDK can leverage it >>> to improve VM-to-VM UDP performance, this will make >>> sure IP fragments will be reassembled once it is >>> received from physical NIC. >>> >>> Signed-off-by: Yi Yang <yangy...@inspur.com<mailto:yangy...@inspur.com>> >>> --- >>> lib/librte_gro/Makefile | 2 + >>> lib/librte_gro/gro_udp4.c | 443 ++++++++++++++++++++++++++++++++ >>> lib/librte_gro/gro_udp4.h | 296 +++++++++++++++++++++ >>> lib/librte_gro/gro_vxlan_udp4.c | 556 >>> ++++++++++++++++++++++++++++++++++++++++ >>> lib/librte_gro/gro_vxlan_udp4.h | 152 +++++++++++ >>> lib/librte_gro/meson.build | 2 +- >>> lib/librte_gro/rte_gro.c | 192 +++++++++++--- >>> lib/librte_gro/rte_gro.h | 8 +- >>> 8 files changed, 1617 insertions(+), 34 deletions(-) >>> create mode 100644 lib/librte_gro/gro_udp4.c >>> create mode 100644 lib/librte_gro/gro_udp4.h >>> create mode 100644 lib/librte_gro/gro_vxlan_udp4.c >>> create mode 100644 lib/librte_gro/gro_vxlan_udp4.h >>> + >>> + >>> +/* >>> + * update the packet length for the flushed packet. >>> + */ >>> +static inline void >>> +update_header(struct gro_udp4_item *item) >>> +{ >>> + struct rte_ipv4_hdr *ipv4_hdr; >>> + struct rte_mbuf *pkt = item->firstseg; >>> + uint16_t frag_offset; >>> + >>> + ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) + >>> + pkt->l2_len); >>> + ipv4_hdr->total_length = rte_cpu_to_be_16(pkt->pkt_len - >>> + pkt->l2_len); >>> + >>> + /* Clear MF bit if it is last fragment */ >>> + if (item->is_last_frag) { >>> + frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset); >>> + ipv4_hdr->fragment_offset = >>> + rte_cpu_to_be_16(frag_offset & >>> ~RTE_IPV4_HDR_MF_FLAG); >>> + } >> >>Need to clear MF bit for non-last fragments, and we also need to clear offset >>value. > >For non-last fragment, MF (More Fragment) bit is necessary, why do you think >we need to clear MF bit for it? Only last fragment should clear MF bit. offset >value must be set correctly because it is a IP fragment. > >> >>> +} >>> + >>> +int32_t >>> +gro_udp4_reassemble(struct rte_mbuf *pkt, >>> + struct gro_udp4_tbl *tbl, >>> + uint64_t start_time) >>> +{ >>> + struct rte_ether_hdr *eth_hdr; >>> + struct rte_ipv4_hdr *ipv4_hdr; >>> + uint16_t udp_dl, ip_dl; >>> + uint16_t ip_id, hdr_len; >>> + uint16_t frag_offset = 0; >>> + uint8_t is_last_frag; >>> + >>> + struct udp4_flow_key key; >>> + uint32_t cur_idx, prev_idx, item_idx; >>> + uint32_t i, max_flow_num, remaining_flow_num; >>> + int cmp; >>> + uint8_t find; >>> + >>> + /* >>> + * Don't process the packet whose UDP header length is not equal >>> + * to 20. >>> + */ >>> + if (unlikely(pkt->l4_len != UDP_HDRLEN)) >>> + return -1; >> >>UDP header is fixed 8-byte. No need to check here. > >Agree, will remove it in v2. > >> >>> + >>> + eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *); >>> + ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len); >>> + hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len; >>> + >>> + /* >>> + * Don't process non-fragment packet. >>> + */ >>> + if (!is_ipv4_fragment(ipv4_hdr)) >>> + return -1; >>> + >>> + /* >>> + * Don't process the packet whose payload length is less than or >>> + * equal to 0. >>> + */ >>> + udp_dl = pkt->pkt_len - hdr_len; >>> + if (udp_dl <= 0) >>> + return -1; >> >>Udp_dl is unit16_t which will not be negative. > >Good catch, I should use int16_t here, will do it in v2. > >> >>> + >>> + ip_dl = rte_be_to_cpu_16(ipv4_hdr->total_length) - pkt->l3_len; >>> + ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id); >>> + frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset); >>> + is_last_frag = ((frag_offset & RTE_IPV4_HDR_MF_FLAG) == 0) ? 1 : 0; >>> + frag_offset = (uint16_t)(frag_offset & RTE_IPV4_HDR_OFFSET_MASK) >>> << 3; >
