Re: kernel frameworks for jumbo frame

[Rin Okuyama <rokuyama%rk.phys.keio.ac.jp@localhost>]

Jason, Maxime, thank you for helpful comments!

On 2019/01/29 18:03, Maxime Villard wrote:
> Le 29/01/2019 à 09:42, Jason Thorpe a écrit :
> > > On Jan 29, 2019, at 10:32 AM, Maxime Villard <max%m00nbsd.net@localhost> wrote:
> > >
> > > - As you said, we have only one cluster size to begin with.
> > > - MEXTMALLOC is here to allocate variable-sized clusters, but it is
> > >   really ugly and should be removed. Same for MEXTADD.
> > > - The error handling of MCLGET() is error-prone.
> > > - The naming of certain functions is confusing, like m_getcl, which
> > >   is one typo away from m_clget.
> >
> > MEXTMALLOC() should probably go away, yes.  However, MEXTADD() and the whole
> > set of "arbitrary external storage" has the potential to be quite useful.
> > Back in the day, I used it to implement a zero-copy data path from
> > disk-to-network in an iSCSI storage appliance.  I know that the sosend_loan
> > stuff has been disabled for a while, but I think it's worth keeping the
> > infrastructure that enables it around.
> >
> > Now, if you're just talking about removing MEXTADD() itself, but keeping the
> > underlying _m_ext_storage infrastructure, fine.
>
> The main reason why I don't like MEXTADD is that the 'ext_free' field of
> _m_ext_storage is in charge of freeing the mbuf itself, and not just its
> storage. Because of that we have to make 'mb_cache' public, and it is now
> hanging around in several drivers. In terms of abstraction, this is bad.
> In terms of diagnostic checks, too, because the drivers don't set MT_FREE
> and m_data to NULL.
>
> But anyway, we can keep MEXTADD and just fix that (and also rename it).

I think we agree to add API something like

On 2019/01/29 17:32, Maxime Villard wrote:
>      int
>      m_addjcl(struct mbuf *m, int how, int size)

or

On 2019/01/29 17:31, Jason Thorpe wrote:
> 	int	mbuf_get_cluster(struct mbuf *m, size_t desired_size);

I would like to write a draft of patch. Here, how should we treat size
argument; (a) cluster size itself (like FreeBSD), or (b) minimum required
size of cluster (like OpenBSD)? IMO, the latter is better; the overhead
should be negligible for modern processors. Otherwise, in addition to

On 2019/01/29 17:31, Jason Thorpe wrote:
> Probably worth having a call that returns the maximum cluster size available:
>
> 	size_t mbuf_max_cluster_size(void);

function, we may need a function which returns a cluster size
corresponding to a required size. Is it too much?

On 2019/01/29 17:31, Jason Thorpe wrote:
> > (2)
> > We do not support maximum MTU other than 1500 or 9000 bytes. However,
> > for example, ixg(4) supports max MTU of 16114, and it is restricted to
> > 4088 for axen(4).
> >
> > FreeBSD handles SIOCSIFMTU ioctl in each driver, whereas OpenBSD added
> > if_hardmtu member to struct ifnet for this purpose.
> >
> >
> > For (2), it seems better for me to switch from ETHERCAP_JUMBO_MTU flag
> > to the OpenBSD way of if_hardmtu. What should we do for (1)?
>
> Centralizing it is always better, with a driver-specific hook as needed.  We can already do that either e.g. ether_ioctl(), which allows the driver to trap SIOCSIFMTU itself if it wants to, before forwarding on to the generic code.  (Maybe it needs to poke a register to enable jumbo frames...)
>
> But if_hardmtu is a bad name, and it seems like it's short-sighted in general to start adding stuff like this willy-nilly.  It think a better approach is to add support for generic interface properties, hidden behind an if_{get,set}_prop_<whatever> API.  Ethernet interfaces would get the standard 1500 ETHERMTU unless, for example, the "ethernet-max-mtu" property were set by the individual driver.
>
> As an aside, I think we should probably change if_media work as properties, as well.

I don't understand where the value of "ethernet-max-mtu" itself is
stored? I don't think it is not so bad idea to have something like
if_maxmtu member in struct ifnet; it can also be used in the future
for other media that support variable MTU.

Thanks,
rin