Re: [PATCH v8 7/8] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 16 Oct 2024 12:07:21 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 15/10/2024 05:23, David Gibson wrote:
> >> +/**
> >> + * tcp_vu_update_check() - Calculate TCP checksum
> >> + * @tapside:	Address information for one side of the flow
> >> + * @iov:	Pointer to the array of IO vectors
> >> + * @iov_used:	Length of the array
> >> + */
> >> +static void tcp_vu_update_check(const struct flowside *tapside,
> >> +			        struct iovec *iov, int iov_used)  
> > AFAICT this is only used for the pcap path.  Rather than filling in
> > the checksum at a different point from normal, I think it would be
> > easier to just clear the no_tcp_csum flag when pcap is enabled.  That
> > would, AFAICT, remove the need for this function entirely.  
> 
> To do that is a little bit complicated because we need to pass the iov array to 
> tcp_fill_headers4()/tcp_fill_headers6() to be able to compute the checksum of the TCP part.
> 
> In tcp_buf, the TCP header and TCP payload are in the same iovec but with tcp_vu they can 
> be split on several iovecs. And if we provide the iovec , theoretically we should not 
> provide the TAP, IP and TCP headers via the parameters as they are in the iovec, but for 
> tcp_buf they have one iovec each, and with tcp_vu they are probably all in the same iovec 
> (the first one). So, again, it can be complicated to extract headers to update them.
> 
> In conclusion, I update the checksum only for VU and in the case of pcap because it is 
> simpler (the same logic applies for udp_update_hdr4()/udp_update_hdr6()).
> 
> I'm open to any suggestion that could do the checksum in 
> udp_update_hdr4()/udp_update_hdr6() (I agree with you, it should be the place to do it) 
> but I don't see an easy and nice way to implement it.

I don't really have a suggestion here, but this is probably the first
use case we met where switching to the pcapng format would make sense,
as we could just mark the checksum as "offloaded" in this case:

  https://ietf-opsawg-wg.github.io/draft-ietf-opsawg-pcap/draft-ietf-opsawg-pcapng.html#section-4.3.1 

On the other hand, it comes with a lot of complexity in my opinion so I
still think that it wouldn't make sense in general.

-- 
Stefano