Bug 19866

Summary:	The mbuf subsystem presently uses a horrible referencing scheme for external
Product:	Base System	Reporter:	Bosko.Milekic <Bosko.Milekic>
Component:	kern	Assignee:	dwmalone
Status:	Closed FIXED
Severity:	Affects Only Me
Priority:	Normal
Version:	5.0-CURRENT
Hardware:	Any
OS:	Any

Description Bosko.Milekic 2000-07-12 14:30:02 UTC

	In Synopsis. There are two different diffs that you should look at:

	* http://24.201.62.9/code/mbuf/mbufref.diff : changes:

	   - mbuf ext_buf referencing is now handled entirely at the mbuf subsystem
	     layer. mbufs referencing the same object are kept on a doubly linked list
	     through their m_ext structures.

           - sendfile(2) interface has been modified to accomodate these changes. The
	     external referencing (increment reference count) routine has been removed.

	   - The following drivers (gigabit ethernet card drivers that allocate and
	     manage their own external jumbo-buffer pools) have been modified to
	     work with the new interface, and bloat has been reduced:
			- if_wb
			- if_ti
			- if_sk
	     As I don't have any of these, I am not able to test them properly. Somebody,
	     please test! :-)

	   - mclref*() macros have been eliminated. They are now useless.

	   - m_ext struct within mbufs has been modified. A new void * is
	     provided "ext_args" to which one may attach pretty much anything
	     and have it (optionally) passed to one's ext_free routine.

	   - various other changes have been made to accomodate the new interface.

	   - small cleanups have been made in MGET and MGETHDR that make the system
	     slightly more efficient but are mainly preparatory for work-to-come on
	     the allocator. Expect another preparatory patch to come in soon.

	(Thanks to Ian Dowse and Mike Silbersack for often useful suggestions)

 -----------

	* http://24.201.62.9/code/mbuf/ian.dowse.SLIP.fix : changes:

	   - This fixes if_sl SLIP interface to use the new referencing scheme. This
	     diff was written and produced by Ian Dowse and the full description is
	     at the above URL. Please apply after having applied the first diff above
	     and test if possible - I don't use SLIP.

Fix: 

Not applicable, please read above.

	Thanks!
How-To-Repeat: 
	Not applicable.

Comment 1 bright 2000-07-15 07:44:33 UTC

This is not how it should be done.

Instead of keeping them in a linked list there should be an int/char *
in the mbuf header that works the same way mclrefcnt does.  Instead
of managing a linked list all one has to do is copy the pointer into
the new mbuf header and increment it, and decrease it on free, when
it's zero the deref code is called.

-- 
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]

Comment 2 bmilekic@dsuper.net 2000-07-15 16:05:03 UTC

	I disagree. Especially since you haven't said why it's better than
	what's proposed.

	Of course, here's the reason why: If you are holding a POINTER to an
	outside reference counter, then you make think
	that you're accomodating things for counters outside the subsystem
	but in reality, if you are allocating objects of dynamic size with
	malloc() at some point, you don't really want to have to allocate and
	manage a reference count scheme for that one object anyway. 
	I don't know about you but I want to be able to do malloc(),
	MEXTADD() the ext_buf to the mbuf, and be ready to go. Then, if
	m_copym ever gets called on my mbuf (or my mbuf is in the chain), I
	want it to be taken care of by itself, such that when the mbuf is
	freed, the ext_buf necessarily won't be. I don't want to have to
	malloc() extra space for a counter.

	So the point is: the mbuf subsystem is supposed to transparently
	manage the reference count scheme for itself and if the ext_free
	routine is called and there is an external reference (you can pass it
	using the ext_args pointer) then you don't necessarily need to free
	the object.

	If you still think it should be otherwise, let me know why and I'll
	change it, but I would like to get this off my back as soon as
	possible.

On Fri, 14 Jul 2000, Alfred Perlstein wrote:

> This is not how it should be done.
> 
> Instead of keeping them in a linked list there should be an int/char *
> in the mbuf header that works the same way mclrefcnt does.  Instead
> of managing a linked list all one has to do is copy the pointer into
> the new mbuf header and increment it, and decrease it on free, when
> it's zero the deref code is called.
> 
> -- 
> -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]


--
 Bosko Milekic  *  Voice/Mobile: 514.865.7738  *  Pager: 514.921.0237
    bmilekic@technokratis.com  *  http://www.technokratis.com/

Comment 3 Sheldon Hearn freebsd_committer

2000-07-17 18:23:33 UTC

Responsible Changed
From-To: freebsd-bugs->dwmalone

David, could you use your proximity to Ian to garner a 
third opinion on this one? :-)

Comment 4 dwmalone freebsd_committer

2000-08-19 10:13:57 UTC

State Changed
From-To: open->closed

A much newer version of Bosko's code has been committed.