Date: Fri, 18 Jun 2004 03:20:45 +0200 From: Thomas Moestl <t.moestl@tu-bs.de> To: Pyun YongHyeon <yongari@kt-is.co.kr> Cc: freebsd-sparc64@freebsd.org Subject: Re: TCP/UDP cksum offload on hme(4) Message-ID: <20040618012045.GF747@timesink.dyndns.org> In-Reply-To: <20040617063022.GA11797@kt-is.co.kr> References: <20040616034520.GB7887@kt-is.co.kr> <20040617024759.GA5610@timesink.dyndns.org> <20040617063022.GA11797@kt-is.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
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@tu-bs.de> http://www.tu-bs.de/~y0015675/
<tmm@FreeBSD.org> http://people.FreeBSD.org/~tmm/
"I couldn't read it because my parents forgot to pay the gravity
bill." -- Calvin and Hobbes
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040618012045.GF747>