Date: Wed, 4 Jul 2018 18:20:25 -0700 (PDT) From: "Rodney W. Grimes" <freebsd@pdx.rh.CN85.dnsmgr.net> To: Andrew Gallatin <gallatin@cs.duke.edu> Cc: rgrimes@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r335967 - head/sys/dev/mxge Message-ID: <201807050120.w651KP5K045633@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <658fb418-5a84-dc91-2cc8-1a503a3d43a6@cs.duke.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 07/04/18 15:46, Rodney W. Grimes wrote: > >> Author: gallatin > >> Date: Wed Jul 4 19:29:06 2018 > >> New Revision: 335967 > >> URL: https://urldefense.proofpoint.com/v2/url?u=https-3A__svnweb.freebsd.org_changeset_base_335967&d=DwICAg&c=imBPVzF25OnBgGmVOlcsiEgHoG1i6YHLR0Sj_gZ4adc&r=Ed-falealxPeqc22ehgAUCLh8zlZbibZLSMWJeZro4A&m=2rIiw5AUJ2ishkBkygGMa_9kr0LJOaonX8ni3BF2BHk&s=MwCt6_IgNah0XklsYThsXFcwZD54Xl78TRlnFXJ4zWs&e= > >> > >> Log: > >> mxge: choose appropriate values for hw tso > >> > >> Modified: > >> head/sys/dev/mxge/if_mxge.c > >> > >> Modified: head/sys/dev/mxge/if_mxge.c > >> ============================================================================== > >> --- head/sys/dev/mxge/if_mxge.c Wed Jul 4 18:54:44 2018 (r335966) > >> +++ head/sys/dev/mxge/if_mxge.c Wed Jul 4 19:29:06 2018 (r335967) > >> @@ -4984,6 +4984,9 @@ mxge_attach(device_t dev) > >> ifp->if_ioctl = mxge_ioctl; > >> ifp->if_start = mxge_start; > >> ifp->if_get_counter = mxge_get_counter; > >> + ifp->if_hw_tsomax = 65536 - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN); > > > > Would not this be more accurate (need to reorder assigns): > > ifp->if_hw_tsomax = ifp->if_hw_tsomaxsegsize - (ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN); > > > >> + ifp->if_hw_tsomaxsegcount = sc->ss[0].tx.max_desc; > >> + ifp->if_hw_tsomaxsegsize = 65536; > > > It seems simpler as-is to me. It is using a magic constant twice, where one has a derived value that is dependent on the value of the other. That is bad and error prone and does not document that one depends on the other. Please fix this. Or at least make 65536 a #define so that it only needs changed one place and clearly shows the interdependence of these values. > Looking around at other drivers, I see > at least one (cxgbe) which does the same thing. Bad examples are usually easy to find. > After doing the grep, I'm more concerned with drivers which may > be setting their tsomaxsegsize incorrectly to be too small and > hurting their performance by causing TCP to chop needlessly > at smaller boundaries which are already enforced by their > busdma tags. PAGE_SIZE, which seems to be the common mistaken > size, won't hurt too much I suppose. But the default of 2K > is probably not very good. > > Drew > > -- Rod Grimes rgrimes@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201807050120.w651KP5K045633>