Date: Sat, 6 Jan 2001 16:54:23 -0800 From: Alfred Perlstein <bright@wintelcom.net> To: Bosko Milekic <bmilekic@technokratis.com> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/musycc musycc.c Message-ID: <20010106165423.I15744@fw.wintelcom.net> In-Reply-To: <003c01c07842$cc648bd0$25cbca18@jehovah>; from bmilekic@technokratis.com on Sat, Jan 06, 2001 at 07:43:05PM -0500 References: <200101062044.f06Kiex42615@freefall.freebsd.org> <20010106132943.E15744@fw.wintelcom.net> <005001c07839$40f06820$25cbca18@jehovah> <20010106161427.G15744@fw.wintelcom.net> <003c01c07842$cc648bd0$25cbca18@jehovah>
next in thread | previous in thread | raw e-mail | index | archive | help
* Bosko Milekic <bmilekic@technokratis.com> [010106 16:41] wrote:
>
> > * Bosko Milekic <bmilekic@technokratis.com> [010106 15:33] wrote:
> > >
> > > I find this incredibly dumb. You're just wasting a check that need not be
> > > made. I know when I have to free m and when not to, it's dumb to have to
> > > check it again in errfree. It's Poul's call, however.
> >
> > You have an exceptional condition, it reduces the amount of code
> > someone reading must digest and it should reduce the amount of code
> > the processor needs to put in the I cache.
>
> The problem with it is that not all error conditions should lead to this
> exit path. For example, if you fail before the allocation of the mbuf loop,
> this is certainly not how you want to error. Also, if you fail before you
> MALLOC those buffers, you certainly don't want to try FREE()ing them. I
> suppose you could turn every single error condition in that function to jump
> to some label at the bottom of the function. If anything, I find _that_ to be
> more obfuscating the code than status quo.
That's a bit much, but it's a lot better to keep the normal codepath
clear and understandable than to have all the error handling inlined.
This is really a matter of policy and how you think, obviously we
disagree, however I'm not saying it _has_ to be this way, just that
I prefer it to be that way. :)
I just would like you to consider my position on the subject.
> > There is also a lot of code that already does this:
> >
> > sosend(), soreceive(), soclose().
>
> Huh? What does sosend, soreceive, soclose have anything to do with this?
> Or am I misunderstanding?
At a certain point more code may be added that must do this cleanup,
it may happen after the mbuf it allocated.
All these functions have goto lables for exceptional conditions:
---
sosend:
release:
sbunlock(&so->so_snd);
out:
if (top)
m_freem(top);
if (control)
m_freem(control);
return (error);
}
---
soreceive:
bad:
if (m)
m_freem(m);
return (error);
---
soclose:
drop:
if (so->so_pcb) {
int error2 = (*so->so_proto->pr_usrreqs->pru_detach)(so);
if (error == 0)
error = error2;
}
discard:
if (so->so_state & SS_NOFDREF)
panic("soclose: NOFDREF");
so->so_state |= SS_NOFDREF;
sofree(so);
splx(s);
return (error);
}
> > What you're doing is giving a place to jump to further along in
> > case any other error conditions come up.
>
> What you are doing by moving the m_free down to the bottom is actually
> growing the number of instructions that are cached. In the first case, where
> you m_free at the top before the jump, you simply m_free in that case. In
> your suggestion, you have the added instructions that account for checking
> whether m is NULL, something you already know before jumping.
Yes, but since it's "shouldn't happen" it's better to put the test
at the end rather than inline it in the code.
--
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]
"I have the heart of a child; I keep it in a jar on my desk."
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?20010106165423.I15744>