Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

[Chuck Silvers]

(moving this discussion from gnats mail to tech-kern...)

On Sun, Feb 01, 2015 at 12:56:51PM -0500, Christos Zoulas wrote:
> On Feb 1, 9:33am, ch...@chuq.com (Chuck Silvers) wrote:
> -- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe
>
> | that is not legal. pmap_growkernel() is not optional for kmem allocations
> | which increase the maximum kmem address in use.
> |
> | and before you try to dive in and make a nowait version of pmap_growkernel(),
> | let's discuss if that's the way we want to resolve this. it's currently
> | illegal to allocate kernel memory (even with nowait/nosleep) while holding a
> | spin mutex. if we want to keep that restriction, then we just need to change
> | this one driver to not do that. if we want to relax that restriction,
> | there a bunch of UVM and pmap changes needed.
>
> I was not planning to do that (make a nowait version of pmap_growkernel)...
> I was questioning exactly that: if a NOWAIT allocation should fail instead
> of grabbing a mutex and only failing when LOCKDEBUG is active. Ideally code
> that always fails in LOCKDEBUG mode, should not "work by accident" in regular
> mode.

sure, adding an DIAGNOSTIC assertion somewhere to catch this would be
an improvement. I'm not sure how you'd actually make the check though.
outside LOCKDEBUG we don't track what mutexes are held.
it might work to say that pmap_growkernel() should only be called with
no interrupts blocked, but I don't think there's an MI way to check that.


> This pmap_growkernel code seems strange to me because it seems that it
> is not allowed to fail (the result is not being checked in 2 of 3 places),
> and perhaps the whole thing should be abstracted...

well, pmap_growkernel() *is* the abstraction. :-)
if you'd like to add a UVM wrapper around it so that we can add MI assertions
in just one place, that would be fine. (it would be nice to have that for
all of the pmap API really.)

the check in uvm_page.c is actually bogus:

uvm_maxkaddr = pmap_growkernel(addr + size);
if (uvm_maxkaddr < (addr + size))
panic("uvm_pageboot_alloc: pmap_growkernel() failed");


but the manpage says:

The pmap_growkernel() function returns the new maximum
kernel virtual address that can be mapped with the
resources it has available. If new resources cannot be
allocated, pmap_growkernel() must panic.

a couple of the pmap implementations are a bit lax about the panic part.

but separate from that, most pmap_growkernel() implmentations use sleep locks,
which is apparently intended to be legitimate. I'd think it would be good if
we continue to allow that, and it would be even better if we removed the
requirement to panic and instead allowed pmap_growkernel() to sleep to wait
for memory. to avoid turning those panic cases into hangs, we might want to
do something like add a check that the pagedaemon thread only sleeps when
it's idle. we could even allow the pagedaemon to sleep when there are pageouts
already pending if we tighten up the drivers to not need to allocate any more
memory to complete an I/O after the strategy call returns, but I suspect that
lots of drivers don't guarantee that currently.

-Chuck

[Christos Zoulas]

On Feb 1, 10:53am, ch...@chuq.com (Chuck Silvers) wrote:
-- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

| sure, adding an DIAGNOSTIC assertion somewhere to catch this would be
| an improvement. I'm not sure how you'd actually make the check though.
| outside LOCKDEBUG we don't track what mutexes are held.
| it might work to say that pmap_growkernel() should only be called with
| no interrupts blocked, but I don't think there's an MI way to check that.

Yes, perhaps this is a worthwhile addition, I am not sure.

| well, pmap_growkernel() *is* the abstraction. :-)
| if you'd like to add a UVM wrapper around it so that we can add MI assertions
| in just one place, that would be fine. (it would be nice to have that for
| all of the pmap API really.)
|
| the check in uvm_page.c is actually bogus:
|
| uvm_maxkaddr = pmap_growkernel(addr + size);
| if (uvm_maxkaddr < (addr + size))
| panic("uvm_pageboot_alloc: pmap_growkernel() failed");
|
|
| but the manpage says:
|
| The pmap_growkernel() function returns the new maximum
| kernel virtual address that can be mapped with the
| resources it has available. If new resources cannot be
| allocated, pmap_growkernel() must panic.
|
| a couple of the pmap implementations are a bit lax about the panic part.
|
| but separate from that, most pmap_growkernel() implmentations use sleep locks,
| which is apparently intended to be legitimate. I'd think it would be good if
| we continue to allow that, and it would be even better if we removed the
| requirement to panic and instead allowed pmap_growkernel() to sleep to wait
| for memory. to avoid turning those panic cases into hangs, we might want to
| do something like add a check that the pagedaemon thread only sleeps when
| it's idle. we could even allow the pagedaemon to sleep when there are pageouts
| already pending if we tighten up the drivers to not need to allocate any more
| memory to complete an I/O after the strategy call returns, but I suspect that
| lots of drivers don't guarantee that currently.

All sounds good to me.

There is also this:

rc = vmem_alloc(vm, size, flags, &va);
if (rc != 0)
return rc;

#ifdef PMAP_GROWKERNEL
/*
* These VA allocations happen independently of uvm_map
* so this allocation must not extend beyond the current limit.
*/
KASSERTMSG(uvm_maxkaddr >= va + size,
"%#"PRIxVADDR" %#"PRIxPTR" %#zx",
uvm_maxkaddr, va, size);
#endif

Which does not explain who failed to do the pmap_growkernel.


And here is the jxgbe.h patch to change the core lock not to block IPL_NET...

christos

Index: ixgbe.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ixgbe/ixgbe.h,v
retrieving revision 1.1
diff -u -u -r1.1 ixgbe.h
--- ixgbe.h 12 Aug 2011 21:55:29 -0000 1.1
+++ ixgbe.h 1 Feb 2015 19:11:06 -0000
@@ -476,7 +476,7 @@


#define IXGBE_CORE_LOCK_INIT(_sc, _name) \
- mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_NET)
+ mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_NONE)
#define IXGBE_CORE_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->core_mtx)
#define IXGBE_TX_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->tx_mtx)
#define IXGBE_RX_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->rx_mtx)


christos

[Masanobu SAITOH]

Hi, all.

I fixed and committed some smallproblem to -current.

The rest is:

- Revert ixgbe_netbsd.c rev. 1.2
- make CORE_LOCK adaptive
- Release spin lock while reinitializing the jumb buffer structure.

Is it OK to commit?


Index: ixgbe.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ixgbe/ixgbe.c,v
retrieving revision 1.17
diff -u -r1.17 ixgbe.c
--- ixgbe.c 4 Feb 2015 04:37:13 -0000 1.17
+++ ixgbe.c 4 Feb 2015 04:38:10 -0000
@@ -3933,12 +3933,16 @@
/* Free current RX buffer structs and their mbufs */
ixgbe_free_receive_ring(rxr);

+ IXGBE_RX_UNLOCK(rxr);
+
/* Now reinitialize our supply of jumbo mbufs. The number
* or size of jumbo mbufs may have changed.
*/
ixgbe_jcl_reinit(&adapter->jcl_head, rxr->ptag->dt_dmat,
2 * adapter->num_rx_desc, adapter->rx_mbuf_sz);

+ IXGBE_RX_LOCK(rxr);
+
/* Configure header split? */
if (ixgbe_header_split)
rxr->hdr_split = TRUE;

Index: ixgbe.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ixgbe/ixgbe.h,v
retrieving revision 1.1

diff -u -r1.1 ixgbe.h

--- ixgbe.h 12 Aug 2011 21:55:29 -0000 1.1

+++ ixgbe.h 4 Feb 2015 04:38:10 -0000

@@ -476,7 +476,7 @@


#define IXGBE_CORE_LOCK_INIT(_sc, _name) \
- mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_NET)

+ mutex_init(&(_sc)->core_mtx, MUTEX_DEFAULT, IPL_SOFTNET)

#define IXGBE_CORE_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->core_mtx)
#define IXGBE_TX_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->tx_mtx)
#define IXGBE_RX_LOCK_DESTROY(_sc) mutex_destroy(&(_sc)->rx_mtx)

Index: ixgbe_netbsd.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/ixgbe/ixgbe_netbsd.c,v
retrieving revision 1.2
diff -u -r1.2 ixgbe_netbsd.c
--- ixgbe_netbsd.c 28 Jan 2015 00:30:25 -0000 1.2
+++ ixgbe_netbsd.c 4 Feb 2015 04:38:10 -0000
@@ -56,7 +56,7 @@

*dtp = NULL;

- if ((dt = kmem_zalloc(sizeof(*dt), KM_NOSLEEP)) == NULL)
+ if ((dt = kmem_zalloc(sizeof(*dt), KM_SLEEP)) == NULL)
return ENOMEM;

dt->dt_dmat = dmat;
@@ -136,19 +136,19 @@
ixgbe_extmem_t *em;
int nseg, rc;

- em = kmem_zalloc(sizeof(*em), KM_NOSLEEP);
+ em = kmem_zalloc(sizeof(*em), KM_SLEEP);

if (em == NULL)
return NULL;

rc = bus_dmamem_alloc(dmat, size, PAGE_SIZE, 0, &em->em_seg, 1, &nseg,
- BUS_DMA_NOWAIT);
+ BUS_DMA_WAITOK);

if (rc != 0)
goto post_zalloc_err;

rc = bus_dmamem_map(dmat, &em->em_seg, 1, size, &em->em_vaddr,
- BUS_DMA_NOWAIT);
+ BUS_DMA_WAITOK);

if (rc != 0)
goto post_dmamem_err;


--
-----------------------------------------------
SAITOH Masanobu (msa...@execsw.org
msa...@netbsd.org)

[Christos Zoulas]

On Feb 4, 1:49pm, msa...@execsw.org (Masanobu SAITOH) wrote:
-- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

| Hi, all.
|
| I fixed and committed some smallproblem to -current.
|
| The rest is:
|
| - Revert ixgbe_netbsd.c rev. 1.2
| - make CORE_LOCK adaptive
| - Release spin lock while reinitializing the jumb buffer structure.
|
| Is it OK to commit?

Does it work with LOCKDEBUG? If yes, go for it.

christos

[Masanobu SAITOH]

Yes, it works with LOCKDEBUG.

ifconfig up works
ifconfig down works
ifconfig up <-> down repeatedly works while forwarding works
reboot works
reboot while forwarding works

>If yes, go for it.
>
> christos
>

[Christos Zoulas]

On Feb 4, 2:16pm, msa...@execsw.org (Masanobu SAITOH) wrote:
-- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

| Yes, it works with LOCKDEBUG.
|
| ifconfig up works
| ifconfig down works
| ifconfig up <-> down repeatedly works while forwarding works
| reboot works
| reboot while forwarding works
|

Go for it, and many thanks for fixing it!

christos

[6b...@6bone.informatik.uni-leipzig.de]

On Wed, 4 Feb 2015, Christos Zoulas wrote:
..
> christos

I have tested NetBSD 7.99.x

The problem is now solved. The PR can be closed.


Regards
Uwe

[Masanobu SAITOH]

On 2015/02/17 19:06, 6b...@6bone.informatik.uni-leipzig.de wrote:
> On Wed, 4 Feb 2015, Christos Zoulas wrote:

> ...

>> christos
>
> I have tested NetBSD 7.99.x
>
> The problem is now solved. The PR can be closed.
>
>
> Regards
> Uwe

Thanks. I'll send the pullup requet to netbsd-7.

[Christos Zoulas]

On Feb 17, 7:16pm, msa...@execsw.org (Masanobu SAITOH) wrote:
-- Subject: Re: PR/49328 CVS commit: src/sys/dev/pci/ixgbe

| On 2015/02/17 19:06, 6b...@6bone.informatik.uni-leipzig.de wrote:
| > On Wed, 4 Feb 2015, Christos Zoulas wrote:
| > ...
| >> christos
| >
| > I have tested NetBSD 7.99.x
| >
| > The problem is now solved. The PR can be closed.
| >
| >
| > Regards
| > Uwe
|
| Thanks. I'll send the pullup requet to netbsd-7.

Remember to pull up everything, including the fix for the locking issue
on the link-layer.

christos