Date: Mon, 11 Jun 2001 15:27:24 -0400 From: Bosko Milekic <bmilekic@technokratis.com> To: Hajimu UMEMOTO <ume@FreeBSD.org> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/netinet ip_output.c Message-ID: <20010611152724.A33314@technokratis.com> In-Reply-To: <200106111838.f5BIcCJ05392@freefall.freebsd.org>; from ume@FreeBSD.org on Mon, Jun 11, 2001 at 11:38:12AM -0700 References: <200106111838.f5BIcCJ05392@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jun 11, 2001 at 11:38:12AM -0700, Hajimu UMEMOTO wrote: > ume 2001/06/11 11:38:12 PDT > > Modified files: > sys/netinet ip_output.c > Log: > This is force commit to mention about previous commit. > > - use 0/8 to specify interface index on multicast get/setsockopt > - make sure to nuke m->m_aux pointer for ipsec, on if_output. > - pass error from ipsec_setsocket() all the way up. > - move ipsec output processing before filtering section. > > Revision Changes Path > 1.127 +1 -1 src/sys/netinet/ip_output.c Since I lost the commit mail in which you made the changes to sys/sys/mbuf.h and sys/kern/uipc_mbuf.c, and since this commit seems to be somewhat related, I'll comment on them here. The commit to sys/sys/mbuf.h severely bloated the mbstat structure. Quads were added to apparently account for trivial things, such as `number of times pullup was done,' and so on. There are several problems with this. First of all, I'm in the process of splitting up the mbstat structure, given the new SMP-friendly mbuf allocator, and this addition is making it worse. Normally, I wouldn't really care, and I would be able to leave the statistics that were added in a `general' version of mbstat that could be separately exported (which is what I've done for things like mh_len, and what not - see the changes to mbuf.h in: http://people.freebsd.org/~bmilekic/code/mb_slab/mb_alloc-CURRENT.diff ) However, given that we're targeted right now at making things more SMP friendly, these additional statistics pose yet another problem. I haven't looked around much to see where you actually put the stats to use, but by simply looking at them, I can tell that if you want to consistently update them, that you'll have to either wrap modifications in the present mbuf_mtx (which will disappear) or you'll have to use atomic() ops, which are relatively expensive considering the fact that all you're dealing with are statistics. I can tell you that the counters for mpfail and mcfail (which I've left in my diff) are equally useless and I would vote for removing them as well. The number of times things like mpfail and mcfail will be incremented will strongly depend on the number of times we have failed to allocate an mbuf or mbuf cluster, and we already account for such failures with the present m_drops counter. I never understood why adding them was useful in any way to begin with. And, if you really insist on leaving them around, can you please consider removing them from mbstat (they have nothing to do with mbuf statistics and if they did, then as I said, m_drops would already cover the failures) and place them somewhere else, perhaps enabled only if the kernel is compiled with debugging on? - especially considering the fact that netstat(1) doesn't even report them. If you do end up doing that, let me know so that I can move mcfail and mpfail there too. Furthermore, the other change to sys/sys/mbuf.h and friends had to do with moving a check from m_free() to the more general MFREE() macro. The check itself bloats the macro (increasing the size of the kernel). Normally, I wouldn't object but from the surrounding comments it appears that it is possible to fix the problem _properly_. The comment claims that the check is there because there is still code that doesn't call M_PULLUP() properly. As opposed to bloating the allocation/free code with the bogus check, how difficult would it be to ensure that M_PULLUP() is called properly? Regards, -- Bosko Milekic bmilekic@technokratis.com To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010611152724.A33314>