Very Computer

btw, I finished today fixing a deadlock condition in the xdr layer
triggered by nfs on highmem machines, here's the fix against 2.4.21pre4,
please apply it now to pre4 or will have to live in my tree with the
other hundred of patches like it happened to some of the patches we're
discussing in this thread.

Explanation is very simple: you _can't_ kmap two times in the context of
a single task (especially if more than one task can run the same code at
the same time). I don't yet have the confirmation that this fixes the
deadlock though (it takes days to reproduce so it will take weeks to
confirm), but I can't see anything else wrong at the moment, and this
remains a genuine highmem deadlock that has to be fixed.  The fix is
optimal, no change unless you run out of kmaps and in turn you can
deadlock, i.e. all the light workloads won't be affected at all.

Note, this was developed on top of 2.4.21pre4aa3, so I had to rework it
to make it apply cleanly to mainline, the version I tested and included
in -aa is different, so this one is untested but if it compiles it will
work like a charm ;).

2.5.62 has the very same deadlock condition in xdr triggered by nfs too.
Andrew, if you're forward porting it yourself like with the filebacked
vma merging feature just let me know so we make sure not to duplicate
effort.

diff -urNp nfs-ref/include/asm-i386/highmem.h nfs/include/asm-i386/highmem.h
--- nfs-ref/include/asm-i386/highmem.h  2003-02-14 07:01:58.000000000 +0100
+++ nfs/include/asm-i386/highmem.h      2003-02-20 21:42:17.000000000 +0100
@@ -56,16 +56,19 @@ extern void kmap_init(void) __init;
 #define PKMAP_NR(virt)  ((virt-PKMAP_BASE) >> PAGE_SHIFT)
 #define PKMAP_ADDR(nr)  (PKMAP_BASE + ((nr) << PAGE_SHIFT))

-extern void * FASTCALL(kmap_high(struct page *page));
+extern void * FASTCALL(kmap_high(struct page *page, int nonblocking));
 extern void FASTCALL(kunmap_high(struct page *page));

-static inline void *kmap(struct page *page)
+#define kmap(page) __kmap(page, 0)
+#define kmap_nonblock(page) __kmap(page, 1)
+
+static inline void *__kmap(struct page *page, int nonblocking)
 {
        if (in_interrupt())
                out_of_line_bug();
        if (page < highmem_start_page)
                return page_address(page);
-       return kmap_high(page);
+       return kmap_high(page, nonblocking);
 }

 static inline void kunmap(struct page *page)
diff -urNp nfs-ref/include/linux/sunrpc/xdr.h nfs/include/linux/sunrpc/xdr.h
--- nfs-ref/include/linux/sunrpc/xdr.h  2003-02-19 01:12:41.000000000 +0100
+++ nfs/include/linux/sunrpc/xdr.h      2003-02-20 21:39:51.000000000 +0100
@@ -137,7 +137,7 @@ void xdr_zero_iovec(struct iovec *, int,
  * XDR buffer helper functions
  */
 extern int xdr_kmap(struct iovec *, struct xdr_buf *, unsigned int);
-extern void xdr_kunmap(struct xdr_buf *, unsigned int);
+extern void xdr_kunmap(struct xdr_buf *, unsigned int, int);
 extern void xdr_shift_buf(struct xdr_buf *, size_t);

 /*
diff -urNp nfs-ref/mm/highmem.c nfs/mm/highmem.c
--- nfs-ref/mm/highmem.c        2002-11-29 02:23:18.000000000 +0100
+++ nfs/mm/highmem.c    2003-02-20 21:45:27.000000000 +0100
@@ -77,7 +77,7 @@ static void flush_all_zero_pkmaps(void)
        flush_tlb_all();
 }

-static inline unsigned long map_new_virtual(struct page *page)
+static inline unsigned long map_new_virtual(struct page *page, int nonblocking)
 {
        unsigned long vaddr;
        int count;
@@ -96,6 +96,9 @@ start:
                if (--count)
                        continue;

+               if (nonblocking)
+                       return 0;
+
                /*
                 * Sleep for somebody else to unmap their entries
                 */
@@ -126,7 +129,7 @@ start:
        return vaddr;
 }

-void *kmap_high(struct page *page)
+void *kmap_high(struct page *page, int nonblocking)
 {
        unsigned long vaddr;

@@ -138,11 +141,15 @@ void *kmap_high(struct page *page)
         */
        spin_lock(&kmap_lock);
        vaddr = (unsigned long) page->virtual;
-       if (!vaddr)
-               vaddr = map_new_virtual(page);
+       if (!vaddr) {
+               vaddr = map_new_virtual(page, nonblocking);
+               if (!vaddr)
+                       goto out;
+       }
        pkmap_count[PKMAP_NR(vaddr)]++;
        if (pkmap_count[PKMAP_NR(vaddr)] < 2)
                BUG();
+ out:
        spin_unlock(&kmap_lock);
        return (void*) vaddr;
 }
diff -urNp nfs-ref/net/sunrpc/xdr.c nfs/net/sunrpc/xdr.c
--- nfs-ref/net/sunrpc/xdr.c    2002-11-29 02:23:23.000000000 +0100
+++ nfs/net/sunrpc/xdr.c        2003-02-20 21:39:51.000000000 +0100
@@ -180,7 +180,7 @@ int xdr_kmap(struct iovec *iov_base, str
 {
        struct iovec    *iov = iov_base;
        struct page     **ppage = xdr->pages;
-       unsigned int    len, pglen = xdr->page_len;
+       unsigned int    len, pglen = xdr->page_len, first_kmap;

        len = xdr->head[0].iov_len;
        if (base < len) {
@@ -203,9 +203,17 @@ int xdr_kmap(struct iovec *iov_base, str
                ppage += base >> PAGE_CACHE_SHIFT;
                base &= ~PAGE_CACHE_MASK;
        }
+       first_kmap = 1;
        do {
                len = PAGE_CACHE_SIZE;
-               iov->iov_base = kmap(*ppage);
+               if (first_kmap) {
+                       first_kmap = 0;
+                       iov->iov_base = kmap(*ppage);
+               } else {
+                       iov->iov_base = kmap_nonblock(*ppage);
+                       if (!iov->iov_base)
+                               goto out;
+               }
                if (base) {
                        iov->iov_base += base;
                        len -= base;
@@ -223,20 +231,23 @@ map_tail:
                iov->iov_base = (char *)xdr->tail[0].iov_base + base;
                iov++;
        }
+ out:
        return (iov - iov_base);
 }

-void xdr_kunmap(struct xdr_buf *xdr, unsigned int base)
+void xdr_kunmap(struct xdr_buf *xdr, unsigned int base, int niov)
 {
        struct page     **ppage = xdr->pages;
        unsigned int    pglen = xdr->page_len;

        if (!pglen)
                return;
-       if (base > xdr->head[0].iov_len)
+       if (base >= xdr->head[0].iov_len)
                base -= xdr->head[0].iov_len;
-       else
+       else {
+               niov--;
                base = 0;
+       }

        if (base >= pglen)
                return;
@@ -250,7 +261,11 @@ void xdr_kunmap(struct xdr_buf *xdr, uns
                 * we bump pglen here, and just subtract PAGE_CACHE_SIZE... */
                pglen += base & ~PAGE_CACHE_MASK;
        }
-       for (;;) {
+       /*
+        * In case we could only do a partial xdr_kmap, all remaining iovecs
+        * refer to pages. Otherwise we detect the end through pglen.
+        */
+       for (; niov; niov--) {
                flush_dcache_page(*ppage);
                kunmap(*ppage);
                if (pglen <= PAGE_CACHE_SIZE)
@@ -322,9 +337,22 @@ void
 xdr_shift_buf(struct xdr_buf *xdr, size_t len)
 {
        struct iovec iov[MAX_IOVEC];
-       unsigned int nr;
+       unsigned int nr, len_part, n, skip;
+
+       skip = 0;
+       do {
+
+               nr = xdr_kmap(iov, xdr, skip);
+
+               len_part = 0;
+               for (n = 0; n < nr; n++)
+                       len_part += iov[n].iov_len;
+
+               xdr_shift_iovec(iov, nr, len_part);
+
+               xdr_kunmap(xdr, skip, nr);

-       nr = xdr_kmap(iov, xdr, 0);
-       xdr_shift_iovec(iov, nr, len);
-       xdr_kunmap(xdr, 0);
+               skip += len_part;
+               len -= len_part;
+       } while (len);
 }
diff -urNp nfs-ref/net/sunrpc/xprt.c nfs/net/sunrpc/xprt.c
--- nfs-ref/net/sunrpc/xprt.c   2003-01-29 06:14:32.000000000 +0100
+++ nfs/net/sunrpc/xprt.c       2003-02-20 21:39:51.000000000 +0100
@@ -226,23 +226,34 @@ xprt_sendmsg(struct rpc_xprt *xprt, stru
        /* Dont repeat bytes */
        skip = req->rq_bytes_sent;
        slen = xdr->len - skip;
-       niov = xdr_kmap(niv, xdr, skip);
+       oldfs = get_fs(); set_fs(get_ds());
+       do {
+               unsigned int slen_part, n;

-       msg.msg_flags   = MSG_DONTWAIT|MSG_NOSIGNAL;
-       msg.msg_iov     = niv;
-       msg.msg_iovlen  = niov;
-       msg.msg_name    = (struct sockaddr *) &xprt->addr;
-       msg.msg_namelen = sizeof(xprt->addr);
-       msg.msg_control = NULL;
-       msg.msg_controllen = 0;
+               niov = xdr_kmap(niv, xdr, skip);

-       oldfs = get_fs(); set_fs(get_ds());
-       clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
-       result = sock_sendmsg(sock, &msg, slen);
+               msg.msg_flags   = MSG_DONTWAIT|MSG_NOSIGNAL;
+               msg.msg_iov     = niv;
+               msg.msg_iovlen  = niov;
+               msg.msg_name    = (struct sockaddr *) &xprt->addr;
+               msg.msg_namelen = sizeof(xprt->addr);
+               msg.msg_control = NULL;
+               msg.msg_controllen = 0;
+
+               slen_part = 0;
+               for (n = 0; n < niov; n++)
+                       slen_part += niv[n].iov_len;
+
+               clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
+               result = sock_sendmsg(sock, &msg, slen_part);
+
+               xdr_kunmap(xdr, skip, niov);
+
+               skip += slen_part;
+               slen -= slen_part;
+       } while (result >= 0 && slen);
        set_fs(oldfs);

-       xdr_kunmap(xdr, skip);
-
        dprintk("RPC:      xprt_sendmsg(%d) = %d\n", slen, result);

        if (result >= 0)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

        Jeff

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

     > One should also consider kmap_atomic...  (bcrl suggest)

The problem is that sendmsg() can sleep. kmap_atomic() isn't really
appropriate here.

Cheers,
  Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quote:> Explanation is very simple: you _can't_ kmap two times in the context of
> a single task (especially if more than one task can run the same code at
> the same time). I don't yet have the confirmation that this fixes the
> deadlock though (it takes days to reproduce so it will take weeks to
> confirm), but I can't see anything else wrong at the moment, and this
> remains a genuine highmem deadlock that has to be fixed.  The fix is
> optimal, no change unless you run out of kmaps and in turn you can
> deadlock, i.e. all the light workloads won't be affected at all.

What we did was set up a "kmap reservation", which used an atomic_dec()
+ wait_event() to reschedule the task until it could get enough kmaps
to satisfy the request without deadlocking (i.e. exceeding the kmap cap
which we conservitavely set at 3/4 of all kmap space).

A single "server" task could exceed the kmap cap by enough to satisfy the
maximum possible request size, so that a single system with both clients
and servers can always make forward progress even in the face of clients
trying to kmap more than the total amount of kmap space.

This works for us because we are the only consumer of huge amounts of kmaps
on our systems, but it would be nice to have a generic interface to do that
so that multiple apps don't deadlock against each other (e.g. NFS + Lustre).

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

the only option would be the atomic and at the same time persistent
kmaps in the process address space that don't work well with threads...
but again this is 2.4 and we miss it even in 2.5 because of the troubles
they generate.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quote:> A single "server" task could exceed the kmap cap by enough to satisfy the
> maximum possible request size, so that a single system with both clients
> and servers can always make forward progress even in the face of clients
> trying to kmap more than the total amount of kmap space.

> This works for us because we are the only consumer of huge amounts of kmaps
> on our systems, but it would be nice to have a generic interface to do that
> so that multiple apps don't deadlock against each other (e.g. NFS + Lustre).

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quote:> > This works for us because we are the only consumer of huge amounts of kmaps
> > on our systems, but it would be nice to have a generic interface to do that
> > so that multiple apps don't deadlock against each other (e.g. NFS + Lustre).

> This isn't the problem, if NFS wouldn't be broken it couldn't deadlock
> against Lustre even with your design (assuming you don't fall in the two
> problems mentioned above). But still your design is more fragile and
> less scalable, especially for a generic implementation where you don't
> know how many pages you'll reserve in mean, and you don't know how many
> kmaps entries the architecture can provide to you. But of course with
> kmap_nonblock you'll have to fallback submitting single pages if it
> fails, it's a bit more difficult but it's more robust and optimized IMHO.

This is very similar to ext3 journaling, which requests in advance the
maximum number of journal blocks it might need, and blocks until it can
get them all.

The only problem happens when other parts of the kernel start acquiring
multiple kmaps without using the same reservation/accounting system as us.
Each works fine in isolation, but in combination it fails.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

I understand it may be simpler to do the full reservation, in ext3 you
don't even risk anything because you know how large the pool is, but I
think for these cases the kmap_nonblock is superior because you have
obvious depdency on the architecture and you're not able to use at best
all the kmap pool (and here there's not a transaction that has to be
committed all at once so it's doable).  still in practice it will work
fine in combination of the other safe usages (like kmap_nonblock) if you
reserve few enough pages at time.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quote:> For 2.4.x, Hirokazu Takahashi had a patch which allowed for a safe
> kmap of > 1 page in one call. Appended here as an attachment FYI
> (Marcelo do *not* apply!).

+               } else {
+                       iov->iov_base = kmap_nonblock(*ppage);
+                       if (!iov->iov_base)
+                               goto out;
+               }

should be skipping the map_tail thing?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

    >> For 2.5.x we should rather fix MSG_MORE so that it actually
    >> works instead of messing with hacks to kmap().

     > Is the fixing of MSG_MORE likely to actually happen?

We had better try. The server/knfsd has already converted to sendpage
+ MSG_MORE 8-)

That won't work for 2.4.x though, since that doesn't have support for
sendpage over UDP.

Cheers,
  Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Not to tell you'd need to allocate a big pool of atomic kmaps to do
that, and this would eat hundred megs of virtual address space since
it's replicated per-cpu. This is has even less sense, those machines
where the highmem deadlock triggers eats normal zone big time.

Really, the claim that it can be solved with atomic kmaps doesn't make
any sense to me, nor the fact the sock_sendmsg will not schedule if
called with GFP_ATOMIC. Of course it must not schedule if it can be
called from an irq with priority=GFP_ATOMIC, but this isn't the case
we're discussing here, an irq implicitly just disabled preemption by
design and calling sock_sendmsg from irq isn't really desiderable (even
if technically possible maybe with priority=GFP_ATOMIC according to you)
because it will take some time.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/