tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: kernel frameworks for jumbo frame



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


Home | Main Index | Thread Index | Old Index