TCP/UDP cksum offload on hme(4)

[Thomas Moestl]

On Thu, 2004/06/17 at 15:30:22 +0900, Pyun YongHyeon wrote:
> On Thu, Jun 17, 2004 at 04:48:00AM +0200, Thomas Moestl wrote:
>  > On Wed, 2004/06/16 at 12:45:20 +0900, Pyun YongHyeon wrote:
>  > > +
>  > > +	for(; m && m->m_len == 0; m = m->m_next)
>  > > +		;
>  > > +	if (m == NULL || m->m_len < ETHER_HDR_LEN) {
>  > > +		printf("hme_txcksum: m_len < ETHER_HDR_LEN\n");
>  > > +		return; /* cksum will be corrupted */
>  > > +	}
>  > 
>  > It would probably be best to try as hard as possible to not fail
>  > here. Since the conditions in which we would fail because of mbuf
>  > sizes should rarely be true, maybe a good option would be to just
>  > sacrifice some CPU time and do an m_pullup() then to rectify the mbuf
>  > chain according to our needs (hme_txcksum would need to return a
>  > struct mbuf * for that)?
>  > 
> Because we will have ALTQ feature in near future, the driver
> can't call IF_PREPEND anymore. So if m_pullup() failed it would
> be dropped. I thought, we may not encounter such mbuf fragmentation
> in real environments.(I didn't see any of these fragment condition
> on my setup.)
> If we use m_pullup() it will make ALTQ-enabled hme driver useless.

As far as I know, the latest consensus was that there would be an
equivalent function/macro even with ALTQ; otherwise, the driver would
require some massive rework, as we depend on the prepend functionality
right now (because we cannot know if we have a sufficient amount of
free descriptors in advance).
On second thought, though, I agree that adding more complexity to
handle this case may be overkill, and that handling m_pullup()
failures could get a bit yucky.

> 
>  > > +
>  > > +	hlen = ip->ip_hl << 2;
>  > > +	pktlen -= sizeof(struct ether_header);
>  > > +	if (hlen < sizeof(struct ip))
>  > > +		return;
>  > > +	if (ntohs(ip->ip_len) < hlen)
>  > > +		return;
>  > 
>  > This test is a bit redundant with the TCP/UDP ones below.
>  > 
> When we have a packet that contains IP header with options plus some
> corrupted TCP/UDP header(i.e. less than the size of the header),
> should the check be performed?

Hmmm, in that case we could not use the checksumming anyway, so it
would not be necessary. But that is, of course, nitpicking :)

> New patch is available at:
> http://www.kr.freebsd.org/~yongari/hme.freebsd.diff2

Just one further remark:

> +		if ((ifp->if_flags & IFF_LINK0) != 0)
> +			hme_csum_features |= CSUM_UDP;
> +		else
> +			hme_csum_features &= ~CSUM_UDP;

Maybe if_hwassist should be changed accordingly in this case, too. It
might be a bit counter-intuitive to need to do an extra SIOCSIFCAP
before this takes effect.


Thanks for your changes! Since people have reported that this patch
works well on PCI hmes, I am planning to commit it soon.

	- Thomas

-- 
Thomas Moestl	<t.moestl at tu-bs.de>	http://www.tu-bs.de/~y0015675/
		<tmm at FreeBSD.org>	http://people.FreeBSD.org/~tmm/
"I couldn't read it because my parents forgot to pay the gravity
 bill."						-- Calvin and Hobbes