On Tue, Jan 18, 2022 at 12:27:21PM -0500, Mike Pattrick wrote:
> On Tue, Jan 18, 2022 at 11:53 AM Flavio Leitner <f...@sysclose.org> wrote:
> >
> > On Mon, Jan 17, 2022 at 09:47:18AM -0500, Mike Pattrick wrote:
> > > On Fri, Jan 14, 2022 at 4:13 PM Flavio Leitner <f...@sysclose.org> 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 <f...@sysclose.org>
> > > > > 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 <f...@sysclose.org>
> > > > > >
> > > > > > Actually, this was initially reported by Ilya.
> > > > > >
> > > > > > > Signed-off-by: Mike Pattrick <m...@redhat.com>
> > > > > > > ---
> > > > > > > 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.
> >
> > Ok, so that means we could update the checksum there, but then I
> > wonder if there are other places where this issue can happen.
> > I mean, if this is an isolated case, then we can fix using full
> > checksum in 2.17 and work on a more robust solution in master.
> > However, if there are more places with the same issue then we
> > may need to go with an approach as I posted earlier (implementing
> > a generic IP csum handling before sending to the port).
> >
> > What do you think?
>
> Given that Ilya indicated that the 2.17 release should be tagged
> today, I think it's reasonable to work towards getting your full
> solution into 2.18.
>
> In the case of all NAT, right now we are recalculating the checksum
> multiple times in packet_set_ipv4_addr alone. Only your patch series
> could address this.
>
> Were you planning on making any changes and reposting? Or do you want
> comments in the already posted series?
I was talking about the patch posted as a reply in this thread:
https://www.mail-archive.com/ovs-dev@openvswitch.org/msg62585.html
Another solution and, perhaps better, is to take out the
dp_packet_hwol_is_ipv4() check before csum and always compute csum
as before. It's not ideal with TSO enabled, but then we can optimize
in 2.18.
What do you think?
For example:
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 33a1a9295..dfb2606a6 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -3402,11 +3402,9 @@ 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)) {
- l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
- l3_hdr->ip_tot_len,
- htons(ip_len));
- }
+ l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum,
+ l3_hdr->ip_tot_len,
+ htons(ip_len));
l3_hdr->ip_tot_len = htons(ip_len);
}
}
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
