[dpdk-dev] [PATCH v6 1/6] ethdev: add Tx preparation

Ananyev, Konstantin konstantin.ananyev at intel.com
Thu Oct 20 00:07:24 CEST 2016


Hi guys,

> >
> > > +static inline int
> > > +rte_phdr_cksum_fix(struct rte_mbuf *m) {
> > > +	struct ipv4_hdr *ipv4_hdr;
> > > +	struct ipv6_hdr *ipv6_hdr;
> > > +	struct tcp_hdr *tcp_hdr;
> > > +	struct udp_hdr *udp_hdr;
> > > +	uint64_t ol_flags = m->ol_flags;
> > > +	uint64_t inner_l3_offset = m->l2_len;
> > > +
> > > +	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> > > +		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > > +
> > > +	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
> > > +		if (ol_flags & PKT_TX_IPV4) {
> > > +			ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
> > > +					inner_l3_offset);
> > > +
> > > +			if (ol_flags & PKT_TX_IP_CKSUM)
> > > +				ipv4_hdr->hdr_checksum = 0;
> > > +
> > > +			udp_hdr = (struct udp_hdr *)((char *)ipv4_hdr + m-
> > >l3_len);
> > > +			udp_hdr->dgram_cksum = rte_ipv4_phdr_cksum(ipv4_hdr,
> > ol_flags);
> > > +		} else {
> > > +			ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
> > > +					inner_l3_offset);
> > > +			/* non-TSO udp */
> > > +			udp_hdr = rte_pktmbuf_mtod_offset(m, struct udp_hdr *,
> > > +					inner_l3_offset + m->l3_len);
> > > +			udp_hdr->dgram_cksum = rte_ipv6_phdr_cksum(ipv6_hdr,
> > ol_flags);
> > > +		}
> > > +	} else if ((ol_flags & PKT_TX_TCP_CKSUM) ||
> > > +			(ol_flags & PKT_TX_TCP_SEG)) {
> > > +		if (ol_flags & PKT_TX_IPV4) {
> > > +			ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
> > > +					inner_l3_offset);
> > > +
> > > +			if (ol_flags & PKT_TX_IP_CKSUM)
> > > +				ipv4_hdr->hdr_checksum = 0;
> > > +
> > > +			/* non-TSO tcp or TSO */
> > > +			tcp_hdr = (struct tcp_hdr *)((char *)ipv4_hdr + m-
> > >l3_len);
> > > +			tcp_hdr->cksum = rte_ipv4_phdr_cksum(ipv4_hdr, ol_flags);
> > > +		} else {
> > > +			ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct ipv6_hdr *,
> > > +					inner_l3_offset);
> > > +			/* non-TSO tcp or TSO */
> > > +			tcp_hdr = rte_pktmbuf_mtod_offset(m, struct tcp_hdr *,
> > > +					inner_l3_offset + m->l3_len);
> > > +			tcp_hdr->cksum = rte_ipv6_phdr_cksum(ipv6_hdr, ol_flags);
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +#endif /* _RTE_PKT_H_ */
> > >
> >
> > The function expects that all the network headers are in the first, and
> > that each of them is contiguous.
> >
> 
> Yes, I see...

Yes it does.
I suppose it is a legitimate restriction  (assumptions) for those who'd like to use that function.
But that's a good point and I suppose we need to state it explicitly in the API comments.

> 
> > Also, I had an interesting remark from Stephen [1] on a similar code.
> > If the mbuf is a clone, it will modify the data of the direct mbuf, which
> > should be read-only. Note that it is likely to happen in a TCP stack,
> > because the packet is kept locally in case it has to be retransmitted.
> > Cloning a mbuf is more efficient than duplicating it.
> >
> > I plan to fix it in virtio code by "uncloning" the headers.
> >
> > [1] http://dpdk.org/ml/archives/dev/2016-October/048873.html

This subject is probably a bit off-topic... 
My position on it - it shouldn't be a PMD responsibility to make these kind of checks.
I think it should be upper layer responsibility to provide to the PMD an mbuf
that can be safely used (and in that case modified) by the driver.
Let say if upper layer would like to use packet clone for TCP retransmissions,
It can easily overcome that problem by cloning only data part of the packet,
and putting L2/L3/L4 headers in a separate (head) mbuf and chaining it with
cloned data mbuf.

Konstantin 




More information about the dev mailing list