svn commit: r344817 - in head/sys: dev/e1000 net

[Keller, Jacob E]

Hi,

So one of the problems is that iflib sets up the buffers, but drivers also have to actually ensure that they configure hardware to use the extra space. This currently means copying that check to every driver. It’s not copied to anywhere except the em driver, which means most of the iflib drivers today would get zero benefit out of enabling CONTIG_MALLOCWORKS.

Further, the define is *not* documented anywhere in the kernel, and the name is confusing so it’s unclear what it’s purpose was for.

Personally, I’d like to see


a)      a better name (though sticking with CONTIG_MALLOCWORKS is ok, since it was already in use)

b)      documented somehow, possibly like an opt_* option like RSS is, and with something which explains why and how to enable it

c)       extend iflib to provide access to the actually selected buffer size, so that iflib drivers don’t need to duplicate this calculation when setting up the receive buffers in hardware

d)      extend iflib to prevent using a larger allocation size than the maximum RX segment size. (i.e. not all hardware supports 9k or 16k jumbo frames, so it’s a bit weird for iflib to allocate buffers this large when they won’t be used).

Thanks,
Jake

From: Eric Joyner [mailto:erj at freebsd.org]
Sent: Tuesday, March 05, 2019 2:06 PM
To: rgrimes at freebsd.org; Matthew Macy <mat.macy at gmail.com>
Cc: src-committers <src-committers at freebsd.org>; svn-src-all <svn-src-all at freebsd.org>; svn-src-head <svn-src-head at freebsd.org>; Keller, Jacob E <jacob.e.keller at intel.com>
Subject: Re: svn commit: r344817 - in head/sys: dev/e1000 net

I'm cc'ing Jake so that he can provide a response.

- Eric

On Tue, Mar 5, 2019 at 1:12 PM Rodney W. Grimes <freebsd at gndrsh.dnsmgr.net<mailto:freebsd at gndrsh.dnsmgr.net>> wrote:
> This represents a misunderstanding of how defines are used. This left
> the option open to the user to enable the use of larger than page size
> buffers as it does enable better performance. Over the course of a
> long uptime memory can get too fragmented. However, this left it open
> to the end consumer.
>
> I'd like to see this reverted with perhaps a better name for the
> define and the addition of an explanatory comment.
> Thanks.
Yes please.

Also, Matt, does it/would it work to stop the memory fragmentation issue
if we push the allocation up to the next page size and just waste the
3k bytes?  Some people might be willing to make that trade off to get
long up times and full 9k jumbo's.


Thanks,
Rod

> -M
>
> On Tue, Mar 5, 2019 at 11:13 AM Eric Joyner <erj at freebsd.org<mailto:erj at freebsd.org>> wrote:
> >
> > Author: erj
> > Date: Tue Mar  5 19:12:51 2019
> > New Revision: 344817
> > URL: https://svnweb.freebsd.org/changeset/base/344817
> >
> > Log:
> >   Remove references to CONTIGMALLOC_WORKS in iflib and em
> >
> >   From Jake:
> >   "The iflib_fl_setup() function tries to pick various buffer sizes based
> >   on the max_frame_size value defined by the parent driver. However, this
> >   code was wrapped under CONTIGMALLOC_WORKS, which was never actually
> >   defined anywhere.
> >
> >   This same code pattern was used in if_em.c, likely trying to match
> >   what iflib uses.
> >
> >   Since CONTIGMALLOC_WORKS is not defined, remove this dead code from
> >   iflib_fl_setup and if_em.c
> >
> >   Given that various iflib drivers appear to be using a similar
> >   calculation, it might be worth making this buffer size a value that the
> >   driver can peek at in the future."
> >
> >   Submitted by: Jacob Keller <jacob.e.keller at intel.com<mailto:jacob.e.keller at intel.com>>
> >   Reviewed by:  shurd@
> >   MFC after:    1 week
> >   Sponsored by: Intel Corporation
> >   Differential Revision:        https://reviews.freebsd.org/D19199
> >
> > Modified:
> >   head/sys/dev/e1000/if_em.c
> >   head/sys/net/iflib.c
> >
> > Modified: head/sys/dev/e1000/if_em.c
> > ==============================================================================
> > --- head/sys/dev/e1000/if_em.c  Tue Mar  5 19:08:37 2019        (r344816)
> > +++ head/sys/dev/e1000/if_em.c  Tue Mar  5 19:12:51 2019        (r344817)
> > @@ -1276,15 +1276,8 @@ em_if_init(if_ctx_t ctx)
> >          */
> >         if (adapter->hw.mac.max_frame_size <= 2048)
> >                 adapter->rx_mbuf_sz = MCLBYTES;
> > -#ifndef CONTIGMALLOC_WORKS
> >         else
> >                 adapter->rx_mbuf_sz = MJUMPAGESIZE;
> > -#else
> > -       else if (adapter->hw.mac.max_frame_size <= 4096)
> > -               adapter->rx_mbuf_sz = MJUMPAGESIZE;
> > -       else
> > -               adapter->rx_mbuf_sz = MJUM9BYTES;
> > -#endif
> >         em_initialize_receive_unit(ctx);
> >
> >         /* Use real VLAN Filter support? */
> >
> > Modified: head/sys/net/iflib.c
> > ==============================================================================
> > --- head/sys/net/iflib.c        Tue Mar  5 19:08:37 2019        (r344816)
> > +++ head/sys/net/iflib.c        Tue Mar  5 19:12:51 2019        (r344817)
> > @@ -2187,17 +2187,8 @@ iflib_fl_setup(iflib_fl_t fl)
> >          */
> >         if (sctx->isc_max_frame_size <= 2048)
> >                 fl->ifl_buf_size = MCLBYTES;
> > -#ifndef CONTIGMALLOC_WORKS
> >         else
> >                 fl->ifl_buf_size = MJUMPAGESIZE;
> > -#else
> > -       else if (sctx->isc_max_frame_size <= 4096)
> > -               fl->ifl_buf_size = MJUMPAGESIZE;
> > -       else if (sctx->isc_max_frame_size <= 9216)
> > -               fl->ifl_buf_size = MJUM9BYTES;
> > -       else
> > -               fl->ifl_buf_size = MJUM16BYTES;
> > -#endif
> >         if (fl->ifl_buf_size > ctx->ifc_max_fl_buf_size)
> >                 ctx->ifc_max_fl_buf_size = fl->ifl_buf_size;
> >         fl->ifl_cltype = m_gettype(fl->ifl_buf_size);
> >
>
>

--
Rod Grimes                                                 rgrimes at freebsd.org<mailto:rgrimes at freebsd.org>