Re: kernel frameworks for jumbo frame

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 29/01/2019 à 05:57, Rin Okuyama a écrit :
> Hi,
>
> I'm working on jumbo frame support for axen(4) and mue(4). However,
> unfortunately, I find that we do not have proper kernel frameworks
> for jumbo frame; there are two problems:
>
> (1)
> We do not have external mbuf cluster capable for receiving jumbo
> frames. We need to use m_devget(9) [ale(4), sk(4), ...], or have
> per driver pool [bge(4), dge(4), ...]. The former has a performance
> problem, whereas the latter makes drivers complicated.
>
> FreeBSD has 4 cluster sizes, MCLBYTES, pagesize, 9KiB, and 16KiB.
> They are usable via m_getjcl(how, type, flags, size):
>
> https://www.freebsd.org/cgi/man.cgi?query=m_getjcl
>
> Here, the "size" argument should be chosen from 4 values above
> (description in man page is misleading).
>
> OpenBSD has 8 cluster sizes, MCLBYTES, MCLBYTES + 2 (ether aligned
> 2KiB buffer), 4, 8, 9, 12, 16, and 64KiB. They are usable via
> MCLGETI(m, how, ifp, len):
>
> http://man.openbsd.org/MCLGETI
>
> Unlike FreeBSD, the "len" argument can be arbitrary; proper cluster
> is chosen automatically. The "ifp" argument is not used.

We have several problems when it comes to clusters.

 - 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.

I propose we do this:

 - Find another name for m_add, or just remove it (only two users).
 - Rename m_clget to m_addjcl, add a 'size' argument to it, and make
   it return -1 on error. MCLGET() will be an alias to it with MCLBYTES
   as size. The callers are not forced to read the return value and
   can still check for M_EXT. We can then smoothly switch them to
   handle the return value instead.
 - Add m_getjcl, same as FreeBSD, and make it call m_addjcl. This should
   solve your problem.
 - Make m_getcl an alias to m_getjcl with MCLBYTES as size.

In short, something like:

	int
	m_addjcl(struct mbuf *m, int how, int size)
	{
		// transform 'm' into a cluster of size 'size'
		// return -1 on error, 0 otherwise
	}

	#define MCLGET(m, how) m_addjcl((m), (how), MCLBYTES)

	struct mbuf *
	m_getjcl(int how, int type, int flags, int size)
	{
		... allocate an mbuf 'm'...

		if (m_addjcl(m, how, size) != 0) {
			m_free(m);
			return NULL;
		}

		return m;
	}

	#define m_getcl(how, type, flags) m_getjcl(how, type, flags, MCLBYTES)

This design should simplify a lot of things: we will be able to remove
MEXTMALLOC, improve the error handling of MCLGET, and have a unified
way to allocate jumbo frames.

Changes I would then like to see (but which don't matter right now):

 - Rename MCLGET to m_addcl, and switch each caller to handle errors via
   the return value. This will be a great improvement.
 - If MEXTADD is still needed, rename it to m_addraw. But normally it
   shouldn't be needed, and we should be able to remove it.

What do you think about that?