Re: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for TCP traffic

Hi,

Thanks for finding these bugs! I'll post an updated version soon (2 patches
with no separate Kconfig patches, one LRO and one eHEA patch). See comments below.

Thanks,
Jan-Bernd

On Monday 30 July 2007 22:32, Andrew Gallatin wrote:
I was working on testing the myri10ge patch, and I ran into a few
problems. I've attached a patch to inet_lro.c to fix some of them,
and a patch to myri10ge.c to show how to use the page based
interface. Both patches are signed off by Andrew Gallatin
<gallatin@xxxxxxxx>

First, the LRO_MAX_PG_HLEN is still a problem. Minimally sized 60
byte frames still cause problems in lro_gen_skb due to skb->len
going negative. Fixed in the attached patch. It may be simpler
to just drop LRO_MAX_PG_HLEN to ETH_ZLEN, but I'm not sure if
that is enough. Are there "smart" NICs which might chop off padding
themselves?

I'd tend to stick to an explicit check as implemented in your patch
for now


Second, you still need to set skb->ip_summed = CHECKSUM_UNNECESSARY
when modified packets are flushed, else the stack will see bad
checksums for packets from CHECKSUM_COMPLETE drivers using the
skb interface. Fixed in the attached patch.

I thought about it... As we do update the TCP checksum for aggregated
packets we could add a second ip_summed field in the net_lro_mgr struct
used for aggregated packets to support HW that does not have any checksum helper
functionality. These drivers could set this ip_summed field to CHECKSUM_NONE,
and thus leave the checksum check to the stack. I'm not sure if these old devices benefit
a lot from LRO. So what do you think?


Fourth, I did some traffic sniffing to try to figure out what's going
on above, and saw tcpdump complain about bad checksums. Have you tried
running tcpdump -s 65535 -vvv? Have you also seen bad checksums?
I seem to see this for both page- and skb-based versions of the driver.


Hmmm, can't confirm that. For our skb-based version I see
correct checksums for aggregated packets and for the page-based version as well.
I used: (tcpdump -i ethX -s 0 -w dump.bin) in combination with ethereal.
Don't see problems as well with your tcpdump command.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [PATCH] Add eeprom_bad_csum_allow module option to e1000.
    ... This is a common enough problem that not including it causes more pain ... the hardware was dead before I tried ajax's patch. ... fact that you did not report it as an argument which is out of place. ... That way users with bad checksums wouldn't just ignore the problem, ...
    (Linux-Kernel)
  • Re: aesni(?) corrupts data on 8.2-BETA1
    ... instead of CBC) taking checksums of all files: ... 8.2-BETA1 without patch without aesni loaded: all checksums ok. ... The patch seems to fix my geli problem on 8.2-BETA1 amd64, ...
    (freebsd-stable)
  • Re: [PATCH 6/7] omfs: add checksumming routines
    ... Subject: [PATCH] omfs: add checksumming routines ... OMFS checksums the metadata of all filesystem objects. ... * Update the header checksums for a dirty inode based on its contents. ...
    (Linux-Kernel)
  • Re: Manually verify Windows patch MD5 checksum?
    ... You want the known good checksums on updates. ... I'd like to be able to confirm that a downloaded patch from Microsoft has ... If I manually download a patch from Microsoft and compute an MD5 ...
    (microsoft.public.windowsupdate)
  • 9_Recommended error codes (specifically return code 5)
    ... * "return code 2" indicates patches are already installed. ... * "return code 25" means a patches requires another patch that is not yet installed. ... With or without using the save option, the patch installation process ... Installing 114008-01... ...
    (SunManagers)