Date: Sun, 1 Feb 2009 16:05:44 +0100 From: Fabian Keil <freebsd-listen@fabiankeil.de> To: Jeff Roberson <jroberson@jroberson.net> Cc: freebsd-net@freebsd.org Subject: Re: mbuf revision, testers/comments wanted. Message-ID: <20090201160544.4f1961b4@fabiankeil.de> In-Reply-To: <20090131125100.N983@desktop> References: <20090131125100.N983@desktop>
next in thread | previous in thread | raw e-mail | index | archive | help
--Sig_/J88GP4GS78NoV.7Fp.3mKvr Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Jeff Roberson <jroberson@jroberson.net> wrote: > http://people.freebsd.org/~jeff/mbuf_ref2.diff > I have been experimenting with different revisions to the mbuf api to=20 > 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=20 > 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 zon= e, 510 + int exttype) 511 +{ 512 + struct mbuf *m; 513 + void *mem; 514 + 515 + switch (size) { 516 + case MCLBYTES: 517 + m =3D m_getcl(how, type, flags); 518 + break; 519 + default: [...] 526 + m =3D 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 =3D=3D 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 >=3D MINCLSIZE) { 907 n =3D m_getcl(how, m->m_type, 0); 908 - nsize =3D MCLBYTES; 909 } else { 910 n =3D m_get(how, m->m_type); 911 - nsize =3D 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 !=3D 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 sto= rage 1614 + * buffer and setup a reference count for said buffer. 1615 + * 1616 + * Arguments: 1617 + * mb The existing mbuf to which to attach the provided buf= fer. 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 freein= g the 1621 + * provided external storage buffer. 1622 + * args A pointer to an argument structure (of any type) to b= e 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 fla= gs, int type) 1634 +{ Is the description for "args" meant to cover both arg1 and arg2? =46rom 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 de= vice 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 0x= c0220000-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 --Sig_/J88GP4GS78NoV.7Fp.3mKvr Content-Type: application/pgp-signature; name=signature.asc Content-Disposition: attachment; filename=signature.asc -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkmFukgACgkQBYqIVf93VJ2xMQCdFjnJHRcbRoWWdFkI/aBZ85Qn xToAoJoNWTgbxw6f+8ySBFOQzN4RFN8i =ZkSl -----END PGP SIGNATURE----- --Sig_/J88GP4GS78NoV.7Fp.3mKvr--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090201160544.4f1961b4>