Re: [dpdk-dev] [PATCH v5 1/4] vhost/lib: add vhost TX offload capabilities in vhost lib

["Liu, Jijiang" <jijiang.liu@intel.com>]

Hi Yunhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, November 13, 2015 3:02 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/4] vhost/lib: add vhost TX offload
> capabilities in vhost lib
> 
> On Thu, Nov 12, 2015 at 08:07:03PM +0800, Jijiang Liu wrote:
> > Add vhost TX offload(CSUM and TSO) support capabilities in vhost lib.
> >
> > Refer to feature bits in Virtual I/O Device (VIRTIO) Version 1.0
> > below,
> >
> > VIRTIO_NET_F_CSUM (0) Device handles packets with partial checksum.
> This "checksum offload" is a common feature on modern network cards.
> > VIRTIO_NET_F_HOST_TSO4 (11) Device can receive TSOv4.
> > VIRTIO_NET_F_HOST_TSO6 (12) Device can receive TSOv6.
> >
> > In order to support these features, and the following changes are
> > added,
> >
> > 1. Extend 'VHOST_SUPPORTED_FEATURES' macro to add the offload
> features negotiation.
> >
> > 2. Dequeue TX offload: convert the fileds in virtio_net_hdr to the related
> fileds in mbuf.
> >
> >
> > Signed-off-by: Jijiang Liu <jijiang.liu@intel.com>
> ...
> > +static void
> > +parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
> > +{
> > +	struct ipv4_hdr *ipv4_hdr;
> > +	struct ipv6_hdr *ipv6_hdr;
> > +	void *l3_hdr = NULL;
> > +	struct ether_hdr *eth_hdr;
> > +	uint16_t ethertype;
> > +
> > +	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
> > +
> > +	m->l2_len = sizeof(struct ether_hdr);
> > +	ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
> > +
> > +	if (ethertype == ETHER_TYPE_VLAN) {
> > +		struct vlan_hdr *vlan_hdr = (struct vlan_hdr *)(eth_hdr + 1);
> > +
> > +		m->l2_len += sizeof(struct vlan_hdr);
> > +		ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
> > +	}
> > +
> > +	l3_hdr = (char *)eth_hdr + m->l2_len;
> > +
> > +	switch (ethertype) {
> > +	case ETHER_TYPE_IPv4:
> > +		ipv4_hdr = (struct ipv4_hdr *)l3_hdr;
> > +		*l4_proto = ipv4_hdr->next_proto_id;
> > +		m->l3_len = (ipv4_hdr->version_ihl & 0x0f) * 4;
> > +		*l4_hdr = (char *)l3_hdr + m->l3_len;
> > +		m->ol_flags |= PKT_TX_IPV4;
> > +		break;
> > +	case ETHER_TYPE_IPv6:
> > +		ipv6_hdr = (struct ipv6_hdr *)l3_hdr;
> > +		*l4_proto = ipv6_hdr->proto;
> > +		m->l3_len = sizeof(struct ipv6_hdr);
> > +		*l4_hdr = (char *)l3_hdr + m->l3_len;
> > +		m->ol_flags |= PKT_TX_IPV6;
> > +		break;
> 
> Note that I'm still not that satisfied with putting all those kind of calculation
> into vhost library.
> 
> Every application requesting TSO and CSUM offload features need setup
> them, so I'm wondering _if_ we can put them into a libraray, say lib_ether,
> and let the application just set few key fields and left others to that lib.
> 
> That could leaves us from touching those chaos, such as TCP and IP, here and
> there. And, that, IMO, would be a more elegant way to leverage hardware
> TSO and CSUM offload features.
> 
> And I guess that might need some efforts and more discussions, so I'm okay
> to left that in later versions. (Hence, I gave my ack).
> 
> (I know little about lib_ether and DPDK hardware TSO settings, so I could be
> wrong, and sorry for that if so)

You suggestion is good, I also think we should add some L2/L3 protocols parse into DPDK libs.
as you said, there need more discussions for this, maybe we can do this in the future.

But now, it is necessary to add parse_ethernet() function here to get the essential information.

> 
> 	--yliu
> 
> > +	default:
> > +		m->l3_len = 0;
> > +		*l4_proto = 0;
> > +		break;
> > +	}
> > +}
> > +
> > +static inline void __attribute__((always_inline))
> > +vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
> > +{
> > +	uint16_t l4_proto = 0;
> > +	void *l4_hdr = NULL;
> > +	struct tcp_hdr *tcp_hdr = NULL;
> > +
> > +	parse_ethernet(m, &l4_proto, &l4_hdr);
> > +	if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > +		if (hdr->csum_start == (m->l2_len + m->l3_len)) {
> > +			switch (hdr->csum_offset) {
> > +			case (offsetof(struct tcp_hdr, cksum)):
> > +				if (l4_proto == IPPROTO_TCP)
> > +					m->ol_flags |= PKT_TX_TCP_CKSUM;
> > +				break;
> > +			case (offsetof(struct udp_hdr, dgram_cksum)):
> > +				if (l4_proto == IPPROTO_UDP)
> > +					m->ol_flags |= PKT_TX_UDP_CKSUM;
> > +				break;
> > +			case (offsetof(struct sctp_hdr, cksum)):
> > +				if (l4_proto == IPPROTO_SCTP)
> > +					m->ol_flags |=
> PKT_TX_SCTP_CKSUM;
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > +		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > +		case VIRTIO_NET_HDR_GSO_TCPV4:
> > +		case VIRTIO_NET_HDR_GSO_TCPV6:
> > +			tcp_hdr = (struct tcp_hdr *)l4_hdr;
> > +			m->ol_flags |= PKT_TX_TCP_SEG;
> > +			m->tso_segsz = hdr->gso_size;
> > +			m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
> > +			break;
> > +		default:
> > +			RTE_LOG(WARNING, VHOST_DATA,
> > +				"unsupported gso type %u.\n", hdr-
> >gso_type);
> > +			break;
> > +		}
> > +	}
> > +}