[dpdk-dev] [PATCH] gro: add UDP GRO and VXLAN UDP GRO support

[yang_y_yi]

At 2020-09-01 12:27:29, "Hu, Jiayu" <jiayu.hu at 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_yi at 163.com <yang_y_yi at 163.com>
>> Sent: Wednesday, July 1, 2020 2:48 PM
>> To: dev at dpdk.org
>> Cc: Hu, Jiayu <jiayu.hu at intel.com>; thomas at monjalon.net;
>> yangyi01 at inspur.com; yang_y_yi at 163.com
>> Subject: [PATCH] gro: add UDP GRO and VXLAN UDP GRO support
>> 
>> From: Yi Yang <yangyi01 at 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 <yangyi01 at 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;