On 10/13/2016 04:16 PM, Yuanhan Liu wrote: > On Thu, Oct 13, 2016 at 04:02:49PM +0200, Olivier MATZ wrote: >> >> >> On 10/13/2016 10:18 AM, Yuanhan Liu wrote: >>> On Mon, Oct 03, 2016 at 11:00:23AM +0200, Olivier Matz wrote: >>>> +/* When doing TSO, the IP length is not included in the pseudo header >>>> + * checksum of the packet given to the PMD, but for virtio it is >>>> + * expected. >>>> + */ >>>> +static void >>>> +virtio_tso_fix_cksum(struct rte_mbuf *m) >>>> +{ >>>> + /* common case: header is not fragmented */ >>>> + if (likely(rte_pktmbuf_data_len(m) >= m->l2_len + m->l3_len + >>>> + m->l4_len)) { >>> ... >>>> + /* replace it in the packet */ >>>> + th->cksum = new_cksum; >>>> + } else { >>> ... >>>> + /* replace it in the packet */ >>>> + *rte_pktmbuf_mtod_offset(m, uint8_t *, >>>> + m->l2_len + m->l3_len + 16) = new_cksum.u8[0]; >>>> + *rte_pktmbuf_mtod_offset(m, uint8_t *, >>>> + m->l2_len + m->l3_len + 17) = new_cksum.u8[1]; >>>> + } >>> >>> The tcp header will always be in the mbuf, right? Otherwise, you can't >>> update the cksum field here. What's the point of introducing the "else >>> clause" then? >> >> Sorry, I don't see the problem you're pointing out here. >> >> What I want to solve here is to support the cases where the mbuf is >> segmented in the middle of the network header (which is probably a rare >> case). > > How it's gonna segmented?
The mbuf is given by the application. So if the application generates a segmented mbuf, it should work. This could happen for instance if the application uses mbuf clones to share the IP/TCP/data part of the mbuf and prepend a specific Ethernet/vlan for different destination. >> In the "else" part, I only access the mbuf byte by byte using the >> rte_pktmbuf_mtod_offset() accessor. An alternative would have been to copy >> the header in a linear buffer, fix the checksum, then copy it again in the >> packet, but there is no mbuf helpers to do these copies for now. > > In the "else" clause, the ip header is still in the mbuf, right? > Why do you have to access it the way like: > > ip_version = *rte_pktmbuf_mtod_offset(m, uint8_t *, > m->l2_len) >> 4; > > Why can't you just use > > iph = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *, m->l2_len); > iph->version_ihl ....; AFAIK, there is no requirement that each network header has to be contiguous in a mbuf segment. Of course, a split in the middle of a network header probably never happens... but we never knows, as it is not forbidden. I think the code should be robust enough to avoid accesses to wrong addresses. Hope it's clear enough :) Thanks Olivier