mbuf revision, testers/comments wanted.

[Fabian Keil]

Jeff Roberson <jroberson at jroberson.net> wrote:

> http://people.freebsd.org/~jeff/mbuf_ref2.diff

> I have been experimenting with different revisions to the mbuf api to 
> improve performance and simplify code.  This patch is the first of
> several proposed steps towards those goals.  The aim of this patch is
> two fold;

> I would appreciate testing feedback from varied workloads to make sure 
> there are no bugs before I go forward with this.  I have tested only
> host oriented networking with a few drivers.  It is not anticipated that
> there will be any significant incompatibilities introduced with this
> round but there is always that possibility.

To compile without changing my kernel config I needed:

--- .zfs/snapshot/2009-02-01/sys/kern/kern_mbuf.c       2009-02-01 14:13:18.678107513 +0100
+++ sys/kern/kern_mbuf.c        2009-02-01 14:24:56.006950417 +0100
@@ -222,7 +222,9 @@
 /*
  * Local prototypes.
  */
+#ifdef INVARIANTS
 static int     mb_ctor_pack(void *mem, int size, void *arg, int how);
+#endif
 static void    mb_dtor_pack(void *mem, int size, void *arg);
 static void    mb_reclaim(void *);
 static int     mb_zinit_pack(void *mem, int size, int how);

I'm not familiar with how mbufs work on FreeBSD,
so a few meta comments are all I got:

1)
   508  +struct mbuf *
   509  +_m_getjcl(int how, short type, int flags, int size, uma_zone_t zone,
   510  +    int exttype)
   511  +{
   512  +       struct mbuf *m;
   513  +       void *mem;
   514  +
   515  +       switch (size) {
   516  +       case MCLBYTES:
   517  +               m = m_getcl(how, type, flags);
   518  +               break;
   519  +       default:
[...]
   526  +               m = m_alloc(zone_ext, 0, how, type, flags);
[...]
   533  +               break;
   534  +       }
   535  +
   536  +       return (m);
   537  +}

Why do you use a switch statement if there are only
two conditions? Is it to be somewhat symmetric to
m_gettype()?

Otherwise the indentation could be partly flattened with:
if (size == MCLBYTES)
	return m_getcl(how, type, flags);
[old default: code here]

2)
   893  @@ -834,8 +753,9 @@
   894   m_dup(struct mbuf *m, int how)
   895   {
[...]
   904  @@ -852,10 +772,8 @@
   905                  /* Get the next new mbuf */
   906                  if (remain >= MINCLSIZE) {
   907                          n = m_getcl(how, m->m_type, 0);
   908  -                       nsize = MCLBYTES;
   909                  } else {
   910                          n = m_get(how, m->m_type);
   911  -                       nsize = MLEN;
   912                  }

The curly brackets are no longer required here.

3)
  1595   static __inline struct mbuf *
  1596  +m_alloc(uma_zone_t zone, int size, int how, short type, int flags)
  1597  +{
[...]
  1603  +       if (type != MT_NOINIT) {
  1604  +               if (m_init(m, zone, size, how, type, flags)) {
  1605  +                       uma_zfree(zone, m);
  1606  +                       return (NULL);
  1607  +               }
  1608  +       }
  1609  +       return (m);
  1610  +}

Why don't you use a single if block here?

4)
  1612  +/*
  1613  + * Configure a provided mbuf to refer to the provided external storage
  1614  + * buffer and setup a reference count for said buffer.
  1615  + *
  1616  + * Arguments:
  1617  + *    mb     The existing mbuf to which to attach the provided buffer.
  1618  + *    buf    The address of the provided external storage buffer.
  1619  + *    size   The size of the provided buffer.
  1620  + *    freef  A pointer to a routine that is responsible for freeing the
  1621  + *           provided external storage buffer.
  1622  + *    args   A pointer to an argument structure (of any type) to be passed
  1623  + *           to the provided freef routine (may be NULL).
  1624  + *    flags  Any other flags to be passed to the provided mbuf.
  1625  + *    type   The type that the external storage buffer should be
  1626  + *           labeled with.
  1627  + *
  1628  + * Returns:
  1629  + *    Nothing.
  1630  + */
  1631  +static __inline void
  1632  +m_extadd(struct mbuf *mb, caddr_t buf, u_int size,
  1633  +    void (*freef)(void *, void *), void *arg1, void *arg2, int flags, int type)
  1634  +{

Is the description for "args" meant to cover both arg1 and arg2?
From the comment alone their differences aren't clear to me,
but then again, that might just be because my lack of mbuf knowledge.
I noticed that this is only relocated code, though.

5)
Finally, I tested the patch on an IBM ThinPad R51. The kernel
hangs on boot, the last messages are (hand transcribed):

iwi0: <Intel(R) PRO/Wireless 2200BG> mem 0xc0214000-0xc0214fff irq 11 at device 2.0 on pci2
iwi0: Reserved 0x1000 bytes for rid 0x10 type 3 at 0xc0214000
iwi0: could not allocate rx mbuf
iwi0: could not allocate Rx ring
bpfdetach: was not attached

Booting without if_iwi.ko works and for em0 I get:

em0: <Intel(R) PRO/1000 Network Connection 6.9.6> port 0x8000-0x803f mem 0xc0220000-0xc023ffff,0xc0200000-0xc020ffff irq 11 at device 1.0 on pci2
em0: Reserved 0x20000 bytes for rid 0x10 type 3 at 0xc0220000
em0: Reserved 0x40 bytes for rid 0x18 type 4 at 0x8000
em0: [FILTER]
em0: bpf attached
em0: Ethernet address: 00:11:...
em0: Could not setup receive structures

Without iwi0 I can't test the patch any further on this system
at the moment, but I can probably give it a try on another system
in the next days. I also wouldn't mind giving another patch a try.

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20090201/dceb1754/signature.pgp