On Fri, Jan 14, 2022 at 4:13 PM Flavio Leitner <[email protected]> wrote: > > On Fri, Jan 14, 2022 at 03:50:52PM -0500, Mike Pattrick wrote: > > On Fri, Jan 14, 2022 at 3:33 PM Flavio Leitner <[email protected]> wrote: > > > > > > > > > Hi Mike, > > > > > > Thanks for working on this issue. > > > > > > On Fri, Jan 14, 2022 at 10:45:35AM -0500, Mike Pattrick wrote: > > > > Formerly when userspace TSO was enabled but with a non-DKDK interface > > > > without support IP checksum offloading, FTP NAT connections would fail > > > > if the packet length changed. This can happen if the packets length > > > > changes during L7 NAT translation, predominantly with FTP. > > > > > > > > Now we correct the IP header checksum if hwol is disabled or if DPDK > > > > will not handle the IP checksum. This fixes the conntrack - IPv4 FTP > > > > Passive with DNAT" test when run with check-system-tso. > > > > > > > > Reported-by: Flavio Leitner <[email protected]> > > > > > > Actually, this was initially reported by Ilya. > > > > > > > Signed-off-by: Mike Pattrick <[email protected]> > > > > --- > > > > lib/conntrack.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > > > index 33a1a9295..1b8a26ac2 100644 > > > > --- a/lib/conntrack.c > > > > +++ b/lib/conntrack.c > > > > @@ -3402,7 +3402,8 @@ handle_ftp_ctl(struct conntrack *ct, const struct > > > > conn_lookup_ctx *ctx, > > > > } > > > > if (seq_skew) { > > > > ip_len = ntohs(l3_hdr->ip_tot_len) + seq_skew; > > > > - if (!dp_packet_hwol_is_ipv4(pkt)) { > > > > + if (!dp_packet_hwol_is_ipv4(pkt) || > > > > + !dp_packet_ip_checksum_valid(pkt)) { > > > > l3_hdr->ip_csum = > > > > recalc_csum16(l3_hdr->ip_csum, > > > > > > > > l3_hdr->ip_tot_len, > > > > htons(ip_len)); > > > > > > The problem is that the current code doesn't include IPv4 csum > > > handling as required by the Linux software ports. > > > > > > The patch above resolves the unit test issue because non-DPDK > > > interfaces will not flag the packet with good IP csum, and then > > > the csum is updated accordingly. However, a packet coming from > > > a physical DPDK port can have that flag set by the PMD, then if > > > it goes through that part the IP csum is not updated, which > > > will cause a problem if that packet is sent out over a Linux > > > software port later.
I was curious about the performance impact of using csum() instead of recalc_csum16(). While setting up this benchmark, I also noticed some ways that we could improve recalc_csum16(). For example, making the function inline and unrolling the while loop. In my test, Performed 2^32 checksum updates with both the original, and my updated recalc_csum16() function, and then 2^32 full ipv4 header checksum calculations with the csum function(). I also tested both with and without clearing the cpu cache between calls. I found that the optimized recalc_csum16() was 1.5x faster then the current implementation, and the current recalc_csum16() implementation was only 2x faster then a full header calculation with csum(). Given these results, and the fact that any time we update the header, we will usually be affecting more then two bytes anyways, I think the performance improvement from using recalc_csum16() instead of csum() isn't so fantastic. -M > > > > This is a good point, I was trying to get to a happy medium without > > repurposing the patchset that you had previously submitted. That way > > this patch could be available immediately, and your more thorough TSO > > patchset could be applied after. > > I see what you did, and I appreciate that. My concern is that it's > not unusual to have packets moving between dpdk and linux ports, so > we might have to visit this issue again, though the unit test is not > failing. > > > I had also prepared a larger patch as part of this work, but scrapped > > it because it was duplicating a lot of the work you had previously > > done. But if you prefer the more substantial solution, we can scrap > > this patch and I can submit the larger one. > > Feel free to duplicate any parts of that RFC if that makes sense. > I think it's not duplicating anything. It's more as doing the > pieces that are more pressing first. > > Let's see the larger one and decide. If it is not suitable, then > maybe as a potential work around we can ignore offloading at that > point and always calculate full IP csum there. > > fbl > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
