Re: mbuf starvation
On Wed, May 12, 1999 at 10:50:42AM -0700, Archie Cobbs wrote:
> > MCLGET(m, M_WAIT);
> > + if (m == 0) {
>
> I think this part of the patch is useless. MCLGET() does not set
> m to NULL when it fails, it simply doesn't set the M_EXT flag.
> ...unless things have changed recently.
No, they apparently haven't. You're absolutely right.
--
Pierre Beyssac p...@enst.fr
To Unsubscribe: send mail to majord...@freebsd.org
with "unsubscribe freebsd-current" in the body of the message
Re: mbuf starvation
On Wed, May 12, 1999 at 01:34:05PM -0700, Matthew Dillon wrote:
> asleep() allows a subroutine deep in the call stack to specify an
> asynchronous blocking condition and then return a temporary failure
> up through the ranks. At the top level, the scheduler sees and acts
> upon the asynchronous blocking condition. Higher level routines do not
So if I get it right, this would give something like the code below.
Is that the idea? What's missing in the asleep/await code to use
them in such a way?
soxxx()
{
for (;;) {
await(&mbuf_slp);
/* code */
error = xxx(&mbuf_slp);
if (error != ENOBUFS)
break;
}
}
m_retry()
{
/* find an mbuf... */
if (/* got an mbuf to return */)
return mbuf;
else {
asleep(&mbuf_slp);
return NULL;
}
}
m_free()
{
/* Free mbuf... */
wakeup(&mbuf_slp);
}
And, unless I'm missing something, we still need to properly check
for NULL return values from m_get and friends.
--
Pierre Beyssac p...@enst.fr
To Unsubscribe: send mail to majord...@freebsd.org
with "unsubscribe freebsd-current" in the body of the message
Re: mbuf starvation
:I think we need to think a bit more about the right semantics before :making such a change. M_WAIT is supposed to mean `I am in process :context and don't mind sleeping in order to get an mbuf, but there is :too much locking going on inside the network stack to be able to :safely sleep without serious risk of deadlock. : :This is the sort of application which would be ideal for Matt's :`asleep' interface. Then, the code could back its way out of any :locks and spls, safely wait for sufficient mbufs to be freed, and then :retry. Even then, it's still possible to deadlock if one process hogs :the entire mbuf pool. It may be necessary to incrementally penalize :processes which do so. : :FWIW, the 4.3 code sleeps in a loop. : :-GAWollman Doing something like this is exactly what was intented for asleep(). The code is not entirely complete, though. Basically the idea is to use asleep() in situations where the system might block but does not normally block in order to avoid both deadlocks and unnecessary code serialization ( due to holding a lock through a blocking situation ). This becomes critically important in SMP models where most of the locks you hold are spinlocks rather then scheduler locks. asleep() allows a subroutine deep in the call stack to specify an asynchronous blocking condition and then return a temporary failure up through the ranks. At the top level, the scheduler sees and acts upon the asynchronous blocking condition. Higher level routines do not need to understand what condition is being blocked on, only that there is some condition being blocked on. With the current model, higher level routines have to assume that lower level routines may block which complicates matters greatly. -Matt Matthew Dillon :-- :Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same :woll...@lcs.mit.edu | O Siem / The fires of freedom :Opinions not those of| Dance in the burning flame :MIT, LCS, CRS, or NSA| - Susan Aglukark and Chad Irschick : : :To Unsubscribe: send mail to majord...@freebsd.org :with "unsubscribe freebsd-current" in the body of the message : To Unsubscribe: send mail to majord...@freebsd.org with "unsubscribe freebsd-current" in the body of the message
Re: mbuf starvation
Pierre Beyssac writes:
> if (resid >= MINCLSIZE) {
> MCLGET(m, M_WAIT);
> + if (m == 0) {
> + error = ENOBUFS;
> + goto release;
> + }
> if ((m->m_flags & M_EXT) == 0)
> goto nopages;
> mlen = MCLBYTES;
I think this part of the patch is useless. MCLGET() does not set
m to NULL when it fails, it simply doesn't set the M_EXT flag.
...unless things have changed recently.
-Archie
___
Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com
To Unsubscribe: send mail to majord...@freebsd.org
with "unsubscribe freebsd-current" in the body of the message
mbuf starvation
< said: > Another big problem is that there's a check in m_retry and friends > that panics when falling short of mbufs! I really believe this does > more harm than good, because it prevents correct calling code > (checking for NULL mbuf pointers) from exiting gracefully with > ENOBUFS... I think we need to think a bit more about the right semantics before making such a change. M_WAIT is supposed to mean `I am in process context and don't mind sleeping in order to get an mbuf, but there is too much locking going on inside the network stack to be able to safely sleep without serious risk of deadlock. This is the sort of application which would be ideal for Matt's `asleep' interface. Then, the code could back its way out of any locks and spls, safely wait for sufficient mbufs to be freed, and then retry. Even then, it's still possible to deadlock if one process hogs the entire mbuf pool. It may be necessary to incrementally penalize processes which do so. FWIW, the 4.3 code sleeps in a loop. -GAWollman -- Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same woll...@lcs.mit.edu | O Siem / The fires of freedom Opinions not those of| Dance in the burning flame MIT, LCS, CRS, or NSA| - Susan Aglukark and Chad Irschick To Unsubscribe: send mail to majord...@freebsd.org with "unsubscribe freebsd-current" in the body of the message
Re: mbuf starvation
On Wed, May 12, 1999 at 05:43:27PM +0200, Stefan Bethke wrote: > I've discussed this with Garett back in September. The reason is quite > simple: unless all cases of not checking for a NULL pointer returned are > fixed (or instrumented with a panic), it is better to fail with a panic > than with some obscure problem later on. Yes, I would agree in the general case, but in that particular case the reasonning is flawed: you panic every time, while there are many cases that currently are handled gracefully by the caller. In other words, you don't leave any choice to the caller. That's bad. IHMO it's not even a good thing in general because mbuf starvations can and _will_ happen as a normal condition, not because of bugs but because of high resource use. It can have its uses for debugging purposes, as a compilation option. -- Pierre Beyssac p...@enst.fr To Unsubscribe: send mail to majord...@freebsd.org with "unsubscribe freebsd-current" in the body of the message
Re: mbuf starvation
Pierre Beyssac wrote: > Another big problem is that there's a check in m_retry and friends > that panics when falling short of mbufs! I really believe this does > more harm than good, because it prevents correct calling code > (checking for NULL mbuf pointers) from exiting gracefully with > ENOBUFS... I've discussed this with Garett back in September. The reason is quite simple: unless all cases of not checking for a NULL pointer returned are fixed (or instrumented with a panic), it is better to fail with a panic than with some obscure problem later on. Stefan -- Mühlendamm 12 | Voice +49-40-256848, +49-177-3504009 D-22089 Hamburg | e-mail: stefan.bet...@hanse.de Germany | s...@freebsd.org To Unsubscribe: send mail to majord...@freebsd.org with "unsubscribe freebsd-current" in the body of the message
mbuf starvation
I'm trying to plug some of the holes not checking for mbuf shortage.
In particular, there are the following unchecked calls to MGET and
friends in /sys/kern/uipc_socket.c:sosend() (see patches below).
Would anyone mind if I commit these? I won't be able to commit
these before next Sunday evening, so if anyone deems these to be
useful, he's welcome to commit before then.
Another big problem is that there's a check in m_retry and friends
that panics when falling short of mbufs! I really believe this does
more harm than good, because it prevents correct calling code
(checking for NULL mbuf pointers) from exiting gracefully with
ENOBUFS...
These could most certainly help with 3.2-RELEASE too. Same problem,
I won't be able to do anything more before Sunday.
--- uipc_socket.c.orig Wed May 5 16:48:57 1999
+++ uipc_socket.c Wed May 12 16:55:27 1999
@@ -497,15 +497,27 @@
} else do {
if (top == 0) {
MGETHDR(m, M_WAIT, MT_DATA);
+ if (m == 0) {
+ error = ENOBUFS;
+ goto release;
+ }
mlen = MHLEN;
m->m_pkthdr.len = 0;
m->m_pkthdr.rcvif = (struct ifnet *)0;
} else {
MGET(m, M_WAIT, MT_DATA);
+ if (m == 0) {
+ error = ENOBUFS;
+ goto release;
+ }
mlen = MLEN;
}
if (resid >= MINCLSIZE) {
MCLGET(m, M_WAIT);
+ if (m == 0) {
+ error = ENOBUFS;
+ goto release;
+ }
if ((m->m_flags & M_EXT) == 0)
goto nopages;
mlen = MCLBYTES;
@@ -617,6 +629,8 @@
flags = 0;
if (flags & MSG_OOB) {
m = m_get(M_WAIT, MT_DATA);
+ if (m == 0)
+ return (ENOBUFS);
error = (*pr->pr_usrreqs->pru_rcvoob)(so, m, flags & MSG_PEEK);
if (error)
goto bad;
--- uipc_mbuf.c.origFri Apr 30 12:33:50 1999
+++ uipc_mbuf.c Wed May 12 17:05:02 1999
@@ -263,10 +263,7 @@
if (m != NULL) {
mbstat.m_wait++;
} else {
- if (i == M_DONTWAIT)
- mbstat.m_drops++;
- else
- panic("Out of mbuf clusters");
+ mbstat.m_drops++;
}
return (m);
}
@@ -291,10 +288,7 @@
if (m != NULL) {
mbstat.m_wait++;
} else {
- if (i == M_DONTWAIT)
- mbstat.m_drops++;
- else
- panic("Out of mbuf clusters");
+ mbstat.m_drops++;
}
return (m);
}
--
Pierre Beyssac p...@enst.fr
To Unsubscribe: send mail to majord...@freebsd.org
with "unsubscribe freebsd-current" in the body of the message
