Re: mbuf LOR
On Thu, 3 Apr 2003, Andrew Gallatin wrote:
Nate Lawson writes:
I was testing some changes to make fxp MPSAFE and got a LOR in allocating
the mbuf cluster and then finally a panic when trying to dereference the
cluster header. Is the mbuf system MPSAFE? Is it ok to call m_getcl
with a device lock held (but not Giant)?
The lock reversal was: 1. fxp softc lock, 2. Giant.
I think the only place it can be coming from is slab_zalloc().
Does the appended (untested) patch help?
BTW, I don't think that there is any need to get Giant for the zone
allocators in the M_NOWAIT case, but I'm not really familar with the
code, and I don't know if the sparc64 uma_small_alloc needs Giant.
BTW, my MPSAFE driver never sees this, but then again, I never
allocate clusters. I use jumbo frames, and carve out my own recv
buffers, so I'm only allocating mbufs, not clusters.
Drew
Index: uma_core.c
===
RCS file: /home/ncvs/src/sys/vm/uma_core.c,v
retrieving revision 1.51
diff -u -r1.51 uma_core.c
--- uma_core.c26 Mar 2003 18:44:53 - 1.51
+++ uma_core.c3 Apr 2003 18:22:14 -
@@ -703,10 +703,14 @@
wait = ~M_ZERO;
if (booted || (zone-uz_flags UMA_ZFLAG_PRIVALLOC)) {
- mtx_lock(Giant);
- mem = zone-uz_allocf(zone,
- zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
- mtx_unlock(Giant);
+ if ((wait M_NOWAIT) == 0) {
+ mtx_lock(Giant);
+ mem = zone-uz_allocf(zone,
+ zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
+ mtx_unlock(Giant);
+ } else {
+ mem = NULL;
+ }
if (mem == NULL) {
ZONE_LOCK(zone);
return (NULL);
You're right about where the problem is (top of stack trace and listing
below). However, your patch causes an immediate panic on boot due to a
NULL deref. I don't think you want it to always return NULL if called
with M_NOWAIT set. :) Other ideas?
slab_zalloc + 0xdf
uma_zone_slab + 0xd8
uma_zalloc_bucket + 0x15d
uma_zalloc_arg + 0x307
malloc
...
m_getcl
(gdb) l *slab_zalloc+0xdf
0xc02f646f is in slab_zalloc (../../../vm/uma_core.c:707).
702 else
703 wait = ~M_ZERO;
704
705 if (booted || (zone-uz_flags UMA_ZFLAG_PRIVALLOC)) {
706 mtx_lock(Giant);
707 mem = zone-uz_allocf(zone,
708 zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
709 mtx_unlock(Giant);
710 if (mem == NULL) {
711 ZONE_LOCK(zone);
-Nate
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]
Giant required by uma (was Re: mbuf LOR)
Nate Lawson writes:
You're right about where the problem is (top of stack trace and listing
below). However, your patch causes an immediate panic on boot due to a
NULL deref. I don't think you want it to always return NULL if called
with M_NOWAIT set. :) Other ideas?
I suppose the only alternative is to do it right and remove Giant
from the uma zone alloc code.
From looking at the code for a little while this morning, it looks
like there are 3 allocators that could be called at this point in the
code:
1) page_alloc():
Calls kmem_malloc(). Should be MPSAFE on NOWAIT allocations. Needs
Giant on WAITOK allocations.
2) obj_alloc():
Calls vm_page_alloc() -- that's MPSAFE.
Calls pmap_qenter() -- I've got no freaking clue if that's MPSAFE on
all platforms. I think it is, since kmem_malloc is MPSAFE it
calls pmap_enter(), but I'm not sure.
uma_small_alloc():
i386 - no uma_small_alloc, no problem
alpha - uma_small_alloc is SMP safe
ia64 - uma_small_alloc should be SMP safe, as it seems to be
doing just the moral equivalent of PHYS_TO_K0SEG() to map the memory
into the kernel.
sparc64: I have no idea..
Drew
slab_zalloc + 0xdf
uma_zone_slab + 0xd8
uma_zalloc_bucket + 0x15d
uma_zalloc_arg + 0x307
malloc
...
m_getcl
(gdb) l *slab_zalloc+0xdf
0xc02f646f is in slab_zalloc (../../../vm/uma_core.c:707).
702 else
703 wait = ~M_ZERO;
704
705 if (booted || (zone-uz_flags UMA_ZFLAG_PRIVALLOC)) {
706 mtx_lock(Giant);
707 mem = zone-uz_allocf(zone,
708 zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
709 mtx_unlock(Giant);
710 if (mem == NULL) {
711 ZONE_LOCK(zone);
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: mbuf LOR
Nate Lawson writes:
You're right about where the problem is (top of stack trace and listing
below). However, your patch causes an immediate panic on boot due to a
NULL deref. I don't think you want it to always return NULL if called
with M_NOWAIT set. :) Other ideas?
The following patch boots passed the basic 'make -j16 buildworld'
test on x86 SMP. As I outlined before, I'm not certain if it is safe
on all platforms.
I'm really eager to see your fxp locking diffs. Even if you're not
comfortable sharing them with the world yet, I'd be interested in
helping out on this.
Drew
Index: vm/uma_core.c
===
RCS file: /home/ncvs/src/sys/vm/uma_core.c,v
retrieving revision 1.51
diff -u -r1.51 uma_core.c
--- vm/uma_core.c 26 Mar 2003 18:44:53 - 1.51
+++ vm/uma_core.c 4 Apr 2003 15:11:34 -
@@ -703,10 +703,15 @@
wait = ~M_ZERO;
if (booted || (zone-uz_flags UMA_ZFLAG_PRIVALLOC)) {
- mtx_lock(Giant);
- mem = zone-uz_allocf(zone,
- zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
- mtx_unlock(Giant);
+ if ((wait M_NOWAIT) == 0) {
+ mtx_lock(Giant);
+ mem = zone-uz_allocf(zone,
+ zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
+ mtx_unlock(Giant);
+ } else {
+ mem = zone-uz_allocf(zone,
+ zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
+ }
if (mem == NULL) {
ZONE_LOCK(zone);
return (NULL);
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: mbuf LOR
On Wed, 2 Apr 2003, Bosko Milekic wrote: BM For what concerns DONTWAIT, you should theoretically be allowed to BM hold a driver lock. But again, there may be a problem. Specifically, BM I see that UMA code has some explicit Giant acquires/frees in certain BM places. When the UMA code gets called from the malloc() code while BM the bucket is being internally allocated in mb_alloc, you may be BM hitting one of those paths. In fact, unless you have a specific Giant BM acquire in the fxp_* routines you list in your trace below, that is BM undoubtedly what is happening because there are no explicit Giant BM acquires in the code path from m_getcl() to malloc(), so they must be BM happening higher up in the call stack. Well, that's interesting. A month ago or so I sent a patch to smp@ with an update to the malloc page documenting the locking requirements. I got only one response. I wonder, how people are expected to correctly use an API, when that API is poorly documented (malloc(9) in this case, but mbuf(9) does not mention 'lock' either). I would ask people knowing the topic to comment on this. As soon as this EC proposal nightmare is over, I may try to write a corresponding section for mbuf(9). Regards, harti Index: malloc.9 === RCS file: /home/ncvs/src/share/man/man9/malloc.9,v retrieving revision 1.30 diff -u -r1.30 malloc.9 --- malloc.924 Feb 2003 05:53:27 - 1.30 +++ malloc.917 Mar 2003 15:06:14 - @@ -147,44 +147,22 @@ to return .Dv NULL if the request cannot be immediately fulfilled due to resource shortage. -Otherwise, the current process may be put to sleep to wait for -resources to be released by other processes. -If this flag is set, -.Fn malloc -will return -.Dv NULL -rather than block. Note that .Dv M_NOWAIT -is defined to be 0, meaning that blocking operation is the default. -Also note that -.Dv M_NOWAIT is required when running in an interrupt context. -.Pp -Programmers should be careful not to confuse -.Dv M_NOWAIT , -the -.Fn malloc -flag, with -.Dv M_DONTWAIT , -an -.Xr mbuf 9 -allocation flag, which is not a valid argument to -.Fn malloc . .It Dv M_WAITOK -Indicates that it is Ok to wait for resources. It is unconveniently -defined as 0 so care should be taken never to compare against this value -directly or try to AND it as a flag. The default operation is to block -until the memory allocation succeeds. +Indicates that it is ok to wait for resources. +If the request cannot be immediately fulfilled the current process is put +to sleep to wait for resources to be released by other processes. The .Fn malloc , .Fn realloc , and .Fn reallocf -functions can only return +functions cannot return .Dv NULL if -.Dv M_NOWAIT +.Dv M_WAITOK is specified. .It Dv M_USE_RESERVE Indicates that the system can dig into its reserve in order to obtain the @@ -194,6 +172,12 @@ programming. .El .Pp +Exactly one of either +.Dv M_WAITOK +or +.Dv M_NOWAIT +must be specified. +.Pp The .Fa type argument is used to perform statistics on memory usage, and for @@ -244,11 +228,37 @@ While it should not be relied upon, this information may be useful for optimizing the efficiency of memory use. .Pp -Malloc flags documented above should -.Em NOT -be used with +Programmers should be careful not to confuse the malloc flags +.Dv M_NOWAIT +and +.Dv M_WAITOK +with the .Xr mbuf 9 -routines as it will cause undesired results. +flags +.Dv M_DONTWAIT +and +.Dv M_TRYWAIT . +.Sh LOCKING CONSIDERATIONS +.Fn malloc , +.Fn realloc +and +.Fn reallocf +may not be called from fast interrupts handlers. +When called from threaded interrupts +.Ar flag +must contain +.Dv M_NOWAIT . +.Pp +.Fn malloc , +.Fn realloc +and +.Fn reallocf +must not be called with +.Dv M_WAITOK +while a mutex other than Giant is held. +Giant may or may not be held when +.Fn free +is called. .Pp Any calls to .Fn malloc -- harti brandt, http://www.fokus.fraunhofer.de/research/cc/cats/employees/hartmut.brandt/private [EMAIL PROTECTED], [EMAIL PROTECTED] ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: mbuf LOR
Nate Lawson writes:
I was testing some changes to make fxp MPSAFE and got a LOR in allocating
the mbuf cluster and then finally a panic when trying to dereference the
cluster header. Is the mbuf system MPSAFE? Is it ok to call m_getcl
with a device lock held (but not Giant)?
The lock reversal was: 1. fxp softc lock, 2. Giant.
Traceback:
zalloc...
malloc()
mb_pop_cont()
mb_alloc()
m_getcl()
fxp_add_rfabuf()
fxp_intr_body()
fxp_intr() -- locks fxp softc
-Nate
I think the only place it can be coming from is slab_zalloc().
Does the appended (untested) patch help?
BTW, I don't think that there is any need to get Giant for the zone
allocators in the M_NOWAIT case, but I'm not really familar with the
code, and I don't know if the sparc64 uma_small_alloc needs Giant.
BTW, my MPSAFE driver never sees this, but then again, I never
allocate clusters. I use jumbo frames, and carve out my own recv
buffers, so I'm only allocating mbufs, not clusters.
Drew
Index: uma_core.c
===
RCS file: /home/ncvs/src/sys/vm/uma_core.c,v
retrieving revision 1.51
diff -u -r1.51 uma_core.c
--- uma_core.c 26 Mar 2003 18:44:53 - 1.51
+++ uma_core.c 3 Apr 2003 18:22:14 -
@@ -703,10 +703,14 @@
wait = ~M_ZERO;
if (booted || (zone-uz_flags UMA_ZFLAG_PRIVALLOC)) {
- mtx_lock(Giant);
- mem = zone-uz_allocf(zone,
- zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
- mtx_unlock(Giant);
+ if ((wait M_NOWAIT) == 0) {
+ mtx_lock(Giant);
+ mem = zone-uz_allocf(zone,
+ zone-uz_ppera * UMA_SLAB_SIZE, flags, wait);
+ mtx_unlock(Giant);
+ } else {
+ mem = NULL;
+ }
if (mem == NULL) {
ZONE_LOCK(zone);
return (NULL);
___
[EMAIL PROTECTED] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to [EMAIL PROTECTED]
mbuf LOR
I was testing some changes to make fxp MPSAFE and got a LOR in allocating the mbuf cluster and then finally a panic when trying to dereference the cluster header. Is the mbuf system MPSAFE? Is it ok to call m_getcl with a device lock held (but not Giant)? The lock reversal was: 1. fxp softc lock, 2. Giant. Traceback: zalloc... malloc() mb_pop_cont() mb_alloc() m_getcl() fxp_add_rfabuf() fxp_intr_body() fxp_intr() -- locks fxp softc -Nate ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: mbuf LOR
Nate Lawson writes: I was testing some changes to make fxp MPSAFE and got a LOR in allocating the mbuf cluster and then finally a panic when trying to dereference the cluster header. Is the mbuf system MPSAFE? Is it ok to call m_getcl with a device lock held (but not Giant)? The lock reversal was: 1. fxp softc lock, 2. Giant. Traceback: zalloc... malloc() mb_pop_cont() mb_alloc() m_getcl() fxp_add_rfabuf() fxp_intr_body() fxp_intr() -- locks fxp softc I thought that this had been fixed. Which zalloc() exactly? Drew ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: mbuf LOR
On Wed, Apr 02, 2003 at 12:39:30PM -0800, Nate Lawson wrote: I was testing some changes to make fxp MPSAFE and got a LOR in allocating the mbuf cluster and then finally a panic when trying to dereference the cluster header. Is the mbuf system MPSAFE? Is it ok to call m_getcl with a device lock held (but not Giant)? Not necessarily if you're doing a TRYWAIT mbuf allocation. Notably, the TRYWAIT allocation may end up in a call to the reclaim routine which calls the protocol drain routines which may end up in an LOR if you're holding some lock that can also be touched from the drain routines. So, in other words, if you do do that you'd have to be very careful. But there is another problem. kmem_malloc() still requires Giant for blockable allocations (TRYWAIT in this case) and so Giant must be held going in there if you're doing a TRYWAIT allocation. For what concerns DONTWAIT, you should theoretically be allowed to hold a driver lock. But again, there may be a problem. Specifically, I see that UMA code has some explicit Giant acquires/frees in certain places. When the UMA code gets called from the malloc() code while the bucket is being internally allocated in mb_alloc, you may be hitting one of those paths. In fact, unless you have a specific Giant acquire in the fxp_* routines you list in your trace below, that is undoubtedly what is happening because there are no explicit Giant acquires in the code path from m_getcl() to malloc(), so they must be happening higher up in the call stack. The lock reversal was: 1. fxp softc lock, 2. Giant. Traceback: zalloc... malloc() mb_pop_cont() mb_alloc() m_getcl() fxp_add_rfabuf() fxp_intr_body() fxp_intr() -- locks fxp softc -Nate -- Bosko Milekic [EMAIL PROTECTED] [EMAIL PROTECTED] ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
