Date: Thu, 2 Jan 2003 15:47:46 -0500 (EST) From: Andrew Gallatin <gallatin@cs.duke.edu> To: Bosko Milekic <bmilekic@unixdaemons.com> Cc: current@freebsd.org Subject: Re: mbuf header bloat ? Message-ID: <15892.42354.150878.922603@grasshopper.cs.duke.edu> In-Reply-To: <20030102152930.A25321@unixdaemons.com> References: <15840.8629.324788.887872@grasshopper.cs.duke.edu> <Pine.BSF.4.21.0211232306410.28833-100000@InterJet.elischer.org> <15841.17237.826666.653505@grasshopper.cs.duke.edu> <20021125130005.A75177@unixdaemons.com> <15842.27547.385354.151541@grasshopper.cs.duke.edu> <20021125160122.A75673@unixdaemons.com> <15842.51914.871010.137070@grasshopper.cs.duke.edu> <20021126191526.B78371@unixdaemons.com> <15892.35521.181516.714686@grasshopper.cs.duke.edu> <20030102152930.A25321@unixdaemons.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Bosko Milekic writes:
>
> On Thu, Jan 02, 2003 at 01:53:53PM -0500, Andrew Gallatin wrote:
> > I'm just tuning up my driver now to catch up to the "recent" interface
> > changes. While there, I went to add a ref count for my driver managed
> > M_EXT clusters. However, m_extadd() does not take a parameter for
> > assignment into mp->m_ext.ref_cnt Eg, I cannot call m_extadd() if I
> > want to use my own refcounter.
> >
> > Is there any chance this could be fixed? O/w, I'll have to
> > avoid calling m_extadd()..
>
> I dunno. I hate the whole story behind the reference counters in the
> mbuf code and I don't know what the correct approach here would be. A
> long long while back I think m_extadd (or its equivalent) did allow
> something like this. Then, we changed it so that counters would be
> allocated by the mbuf code 'transparently' (i.e., so that the Rest Of
> The World didn't have to worry about it). However, not long ago, I
> changed the way reference counters were allocated for mbuf clusters so
> that it would be faster. Counters for other objects are allocated
> with malloc(). The only 'correct' (half-assed) solution I can see is
> to allow m_extadd() to take a 'refcount' argument (again?) - perhaps
> leave a backwards-compatible m_extadd() and add a m_addext() or
> something - and only call malloc() for the counter if refcnt_provided
> == NULL.
>
> To be honest, I don't really like the idea but I don't see a better
> solution. Right now, ref counting for regular mbuf clusters works
> fine and is pretty damn fast, but I don't know how I could make it
> happen for other external buffer types other than the way I just
> described.
There's a second can of worms too. We don't want the mbuf system
freeing externally maintained refcnt pointers. So we need a type
or flag to distinguish them.
I've appended a minimal impact patch that I came up with. It just
adds a new type (EXT_EXTREF) and leaves the m_extadd() api/abi pretty
much unchanged. Callers that want to manage their own refcnt memory
call m_extadd() like this:
mp->m_ext.ref_cnt = &entry->ref_count;
MEXTADD(mp, (void *)entry->jbuf->buf, GM_JLEN,
gm_freebsd_ether_jfree, (void *)entry->jbuf,
0, EXT_EXTREF);
It seems to work just fine, and together with some patches from Alan
Cox for kmem_malloc(), allows me to make my network driver MPSAFE.
I'm still testing for other hidden Giant acquisitions or
GIANT_REQUIRED() assertions in rare codepaths, but its been up for 15
minutes now, and that's 14m 59sec longer than usual ;)
Would you be OK with this or something like it?
Thanks,
Drew
Index: kern/subr_mbuf.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/subr_mbuf.c,v
retrieving revision 1.34
diff -u -r1.34 subr_mbuf.c
--- kern/subr_mbuf.c 27 Nov 2002 04:26:00 -0000 1.34
+++ kern/subr_mbuf.c 2 Jan 2003 19:59:54 -0000
@@ -1062,7 +1062,8 @@
(((uintptr_t)(cl) - (uintptr_t)cl_refcntmap) >> MCLSHIFT)
#define _mext_dealloc_ref(m) \
- free((m)->m_ext.ref_cnt, M_MBUF)
+ if ((m)->m_ext.ext_type != EXT_EXTREF) \
+ free((m)->m_ext.ref_cnt, M_MBUF)
/******************************************************************************
* Internal routines.
@@ -1508,9 +1509,13 @@
m_extadd(struct mbuf *mb, caddr_t buf, u_int size,
void (*freef)(void *, void *), void *args, int flags, int type)
{
+ u_int *ref_cnt = NULL;
- _mext_init_ref(mb, ((type != EXT_CLUSTER) ?
- NULL : &cl_refcntmap[cl2ref(mb->m_ext.ext_buf)]));
+ if (type == EXT_CLUSTER)
+ ref_cnt = &cl_refcntmap[cl2ref(mb->m_ext.ext_buf)];
+ else if (type == EXT_EXTREF)
+ ref_cnt = mb->m_ext.ref_cnt;
+ _mext_init_ref(mb, ref_cnt);
if (mb->m_ext.ref_cnt != NULL) {
mb->m_flags |= (M_EXT | flags);
mb->m_ext.ext_buf = buf;
Index: sys/mbuf.h
===================================================================
RCS file: /home/ncvs/src/sys/sys/mbuf.h,v
retrieving revision 1.109
diff -u -r1.109 mbuf.h
--- sys/mbuf.h 30 Dec 2002 20:22:40 -0000 1.109
+++ sys/mbuf.h 2 Jan 2003 19:40:20 -0000
@@ -173,6 +173,7 @@
#define EXT_NET_DRV 100 /* custom ext_buf provided by net driver(s) */
#define EXT_MOD_TYPE 200 /* custom module's ext_buf type */
#define EXT_DISPOSABLE 300 /* can throw this buffer away w/page flipping */
+#define EXT_EXTREF 400 /* has externally maintained ref_cnt ptr*/
/*
* Flags copied when copying m_pkthdr.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?15892.42354.150878.922603>