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?
Thanks,
fbl
>
> -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
> >
>
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
