[Cluster-devel] [DLM] Fix up memory alloc/kmap

[Cluster-devel] [DLM] Fix up memory alloc/kmap

[Steven Whitehouse]

I've been looking into DLM memory allocation issues and come up with the
following patch. I did a quick go/no-go test and it seems to work ok.

I spotted that DLM doesn't set sk->sk_allocation. Since the sockets are
associated with more than one lockspace, using ls->ls_allocation isn't
an option. Since, however DLM uses sendpage, the amount of memory
which is allocated by the socket code itself will be pretty small,
which is most likely why this issue hasn't been seen before. On that
basis, my suggestion is to use GFP_NOFS, which is the most restrictive
of the GFP modes of which ls->ls_allocation might be set to.

Also there are various places where ls->ls_allocation should have been
used, but had been forgotten. That is fixed up.

In addition kmap was being used incorrectly. The reason that this hasn't
been seen before is that the pages are allocated using ls->ls_allocation
which doesn't allow highmem pages to be returned. I haven't changed that
since although kmap implies a GFP_KERNEL allocation, without highmem
pages, they will be no-ops for now and thus safe. Also sendpage does its
own kmap (if required, which is only if the protocol is emulating
sendpage via sock_no_sendpage) so that the k(un)map pair around that
call can be removed.

I hope that makes some kind of sense, please let me know if you think
I've missed something.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: David Teigland <teigland@redhat.com>
Cc: Christine Caulfield <ccaulfie@redhat.com>

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 3962262..07c6fba 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -295,6 +295,7 @@ static int add_sock(struct socket *sock, struct connection *con)
 	con->sock->sk->sk_write_space = lowcomms_write_space;
 	con->sock->sk->sk_state_change = lowcomms_state_change;
 	con->sock->sk->sk_user_data = con;
+	con->sock->sk->sk_allocation = GFP_NOFS;
 	return 0;
 }
 
@@ -839,6 +840,7 @@ static void sctp_init_assoc(struct connection *con)
 	outmessage.msg_controllen = cmsg->cmsg_len;
 
 	ret = kernel_sendmsg(base_con->sock, &outmessage, iov, 1, len);
+	kunmap(e->page);
 	if (ret < 0) {
 		log_print("Send first packet to node %d failed: %d",
 			  con->nodeid, ret);
@@ -846,15 +848,13 @@ static void sctp_init_assoc(struct connection *con)
 		/* Try again later */
 		clear_bit(CF_CONNECT_PENDING, &con->flags);
 		clear_bit(CF_INIT_PENDING, &con->flags);
-	}
-	else {
+	} else {
 		spin_lock(&con->writequeue_lock);
 		e->offset += ret;
 		e->len -= ret;
 
 		if (e->len == 0 && e->users == 0) {
 			list_del(&e->list);
-			kunmap(e->page);
 			free_entry(e);
 		}
 		spin_unlock(&con->writequeue_lock);
@@ -1272,7 +1272,6 @@ static void send_to_sock(struct connection *con)
 		offset = e->offset;
 		BUG_ON(len == 0 && e->users == 0);
 		spin_unlock(&con->writequeue_lock);
-		kmap(e->page);
 
 		ret = 0;
 		if (len) {
@@ -1294,7 +1293,6 @@ static void send_to_sock(struct connection *con)
 
 		if (e->len == 0 && e->users == 0) {
 			list_del(&e->list);
-			kunmap(e->page);
 			free_entry(e);
 			continue;
 		}
diff --git a/fs/dlm/memory.c b/fs/dlm/memory.c
index 54c14c6..c1775b8 100644
--- a/fs/dlm/memory.c
+++ b/fs/dlm/memory.c
@@ -39,7 +39,7 @@ char *dlm_allocate_lvb(struct dlm_ls *ls)
 {
 	char *p;
 
-	p = kzalloc(ls->ls_lvblen, GFP_KERNEL);
+	p = kzalloc(ls->ls_lvblen, ls->ls_allocation);
 	return p;
 }
 
@@ -57,7 +57,7 @@ struct dlm_rsb *dlm_allocate_rsb(struct dlm_ls *ls, int namelen)
 
 	DLM_ASSERT(namelen <= DLM_RESNAME_MAXLEN,);
 
-	r = kzalloc(sizeof(*r) + namelen, GFP_KERNEL);
+	r = kzalloc(sizeof(*r) + namelen, ls->ls_allocation);
 	return r;
 }
 
@@ -72,7 +72,7 @@ struct dlm_lkb *dlm_allocate_lkb(struct dlm_ls *ls)
 {
 	struct dlm_lkb *lkb;
 
-	lkb = kmem_cache_zalloc(lkb_cache, GFP_KERNEL);
+	lkb = kmem_cache_zalloc(lkb_cache, ls->ls_allocation);
 	return lkb;
 }
 

[Cluster-devel] [DLM] Fix up memory alloc/kmap

[Steven Whitehouse]

This is an updated version of the previous patch with one place that was
missed being fixed up.

I've been looking into DLM memory allocation issues and come up with the
following patch. I did a quick go/no-go test and it seems to work ok.

I spotted that DLM doesn't set sk->sk_allocation. Since the sockets are
associated with more than one lockspace, using ls->ls_allocation isn't
an option. Since, however DLM uses sendpage, the amount of memory
which is allocated by the socket code itself will be pretty small,
which is most likely why this issue hasn't been seen before. On that
basis, my suggestion is to use GFP_NOFS, which is the most restrictive
of the GFP modes of which ls->ls_allocation might be set to.

Also there are various places where ls->ls_allocation should have been
used, but had been forgotten. That is fixed up.

In addition kmap was being used incorrectly. The reason that this hasn't
been seen before is that the pages are allocated using ls->ls_allocation
which doesn't allow highmem pages to be returned. I haven't changed that
since although kmap implies a GFP_KERNEL allocation, without highmem
pages, they will be no-ops for now and thus safe. Also sendpage does its
own kmap (if required, which is only if the protocol is emulating
sendpage via sock_no_sendpage) so that the k(un)map pair around that
call can be removed.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: David Teigland <teigland@redhat.com>
Cc: Christine Caulfield <ccaulfie@redhat.com>

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 3962262..07c6fba 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -295,6 +295,7 @@ static int add_sock(struct socket *sock, struct connection *con)
 	con->sock->sk->sk_write_space = lowcomms_write_space;
 	con->sock->sk->sk_state_change = lowcomms_state_change;
 	con->sock->sk->sk_user_data = con;
+	con->sock->sk->sk_allocation = GFP_NOFS;
 	return 0;
 }
 
@@ -839,6 +840,7 @@ static void sctp_init_assoc(struct connection *con)
 	outmessage.msg_controllen = cmsg->cmsg_len;
 
 	ret = kernel_sendmsg(base_con->sock, &outmessage, iov, 1, len);
+	kunmap(e->page);
 	if (ret < 0) {
 		log_print("Send first packet to node %d failed: %d",
 			  con->nodeid, ret);
@@ -846,15 +848,13 @@ static void sctp_init_assoc(struct connection *con)
 		/* Try again later */
 		clear_bit(CF_CONNECT_PENDING, &con->flags);
 		clear_bit(CF_INIT_PENDING, &con->flags);
-	}
-	else {
+	} else {
 		spin_lock(&con->writequeue_lock);
 		e->offset += ret;
 		e->len -= ret;
 
 		if (e->len == 0 && e->users == 0) {
 			list_del(&e->list);
-			kunmap(e->page);
 			free_entry(e);
 		}
 		spin_unlock(&con->writequeue_lock);
@@ -1272,7 +1272,6 @@ static void send_to_sock(struct connection *con)
 		offset = e->offset;
 		BUG_ON(len == 0 && e->users == 0);
 		spin_unlock(&con->writequeue_lock);
-		kmap(e->page);
 
 		ret = 0;
 		if (len) {
@@ -1294,7 +1293,6 @@ static void send_to_sock(struct connection *con)
 
 		if (e->len == 0 && e->users == 0) {
 			list_del(&e->list);
-			kunmap(e->page);
 			free_entry(e);
 			continue;
 		}
diff --git a/fs/dlm/memory.c b/fs/dlm/memory.c
index 54c14c6..c1775b8 100644
--- a/fs/dlm/memory.c
+++ b/fs/dlm/memory.c
@@ -39,7 +39,7 @@ char *dlm_allocate_lvb(struct dlm_ls *ls)
 {
 	char *p;
 
-	p = kzalloc(ls->ls_lvblen, GFP_KERNEL);
+	p = kzalloc(ls->ls_lvblen, ls->ls_allocation);
 	return p;
 }
 
@@ -57,7 +57,7 @@ struct dlm_rsb *dlm_allocate_rsb(struct dlm_ls *ls, int namelen)
 
 	DLM_ASSERT(namelen <= DLM_RESNAME_MAXLEN,);
 
-	r = kzalloc(sizeof(*r) + namelen, GFP_KERNEL);
+	r = kzalloc(sizeof(*r) + namelen, ls->ls_allocation);
 	return r;
 }
 
@@ -72,7 +72,7 @@ struct dlm_lkb *dlm_allocate_lkb(struct dlm_ls *ls)
 {
 	struct dlm_lkb *lkb;
 
-	lkb = kmem_cache_zalloc(lkb_cache, GFP_KERNEL);
+	lkb = kmem_cache_zalloc(lkb_cache, ls->ls_allocation);
 	return lkb;
 }
 
diff --git a/fs/dlm/midcomms.c b/fs/dlm/midcomms.c
index 07ac709..f3396c6 100644
--- a/fs/dlm/midcomms.c
+++ b/fs/dlm/midcomms.c
@@ -112,7 +112,7 @@ int dlm_process_incoming_buffer(int nodeid, const void *base,
 		   ordinary messages). */
 
 		if (msglen > sizeof(__tmp) && p == &__tmp.p) {
-			p = kmalloc(dlm_config.ci_buffer_size, GFP_KERNEL);
+			p = kmalloc(dlm_config.ci_buffer_size, GFP_NOFS);
 			if (p == NULL)
 				return ret;
 		}

[Cluster-devel] Re: [DLM] Fix up memory alloc/kmap

[David Teigland]

On Tue, Nov 11, 2008 at 11:02:57AM +0000, Steven Whitehouse wrote:
> In addition kmap was being used incorrectly. The reason that this hasn't
> been seen before is that the pages are allocated using ls->ls_allocation
> which doesn't allow highmem pages to be returned. I haven't changed that
> since although kmap implies a GFP_KERNEL allocation, without highmem
> pages, they will be no-ops for now and thus safe. Also sendpage does its
> own kmap (if required, which is only if the protocol is emulating
> sendpage via sock_no_sendpage) so that the k(un)map pair around that
> call can be removed.

Sorry, I don't understand this, it's late and I'm probably being dense...

- We can remove kmap/kunmap from send_to_sock() because...

- We can remove kmap/kunmap from sctp_init_assoc() because...

- We cannot remove kmap/kunmap from dlm_lowcomms_get_buffer/
  dlm_lowcomms_commit_buffer because...

[Cluster-devel] Re: [DLM] Fix up memory alloc/kmap

[Steven Whitehouse]

Hi,

On Wed, 2008-11-12 at 17:14 -0600, David Teigland wrote:
> On Tue, Nov 11, 2008 at 11:02:57AM +0000, Steven Whitehouse wrote:
> > In addition kmap was being used incorrectly. The reason that this hasn't
> > been seen before is that the pages are allocated using ls->ls_allocation
> > which doesn't allow highmem pages to be returned. I haven't changed that
> > since although kmap implies a GFP_KERNEL allocation, without highmem
> > pages, they will be no-ops for now and thus safe. Also sendpage does its
> > own kmap (if required, which is only if the protocol is emulating
> > sendpage via sock_no_sendpage) so that the k(un)map pair around that
> > call can be removed.
> 
> Sorry, I don't understand this, it's late and I'm probably being dense...
> 
> - We can remove kmap/kunmap from send_to_sock() because...
> 
> - We can remove kmap/kunmap from sctp_init_assoc() because...
> 
> - We cannot remove kmap/kunmap from dlm_lowcomms_get_buffer/
>   dlm_lowcomms_commit_buffer because...
> 

kmap is one of the odder kernel interfaces, and it also has the extra
confusion of having a different interface between the atomic and
"normal" versions of itself. Worse still, due to the void pointer, you
don't get a warning that you got it wrong until you try to unmap the
page in question and it fails.

So here is a quick run down of the rules, and perhaps it will make more
sense after this.... firstly kmap of pages which are not highmem pages
will always be a no-op, so unless the pages which are being used have
been allocated with __GFP_HIGHMEM, then kmap will always be a no-op.
Both GFP_KERNEL and GFP_NOFS do not imply __GFP_HIGHMEM (see gfp.h) so
that you'll never need to kmap pages allocated in this way.

Now most kernel calls require that high mem pages are mapped before they
are called, but this is not the case with sendpage. In general the data
will not be even looked at by the kernel, but the page gets added to the
page vector of the skb for the outgoing packet, and its up to the device
driver to do any mapping required. If there is an exception to that, its
probably due to netfilter inspecting the packets.

kmap itself works by (when required, due to a page being in highmem)
allocating (GFP_KERNEL) a low memory page and copying the data into it.
It thus means that every time you see a kmap() there is a potential
GFP_KERNEL allocation, and the point that I was making above is that
(assuming I've read the code correctly) that the GFP_KERNEL allocation
will never happen, because the page can never be a highmem page anyway.

I did consider the idea of using kmap_atomic() instead. In that case
instead of allocating a page, it uses one of two existing pre-allocated
pages (per CPU) to copy the data into temporarily. The gotcha in this
case is that because the pages are per-CPU, you must not schedule
between the kmap/kunmap pair and you also have to be careful not to have
a code path where the same kmap atomic page can be used twice. Since in
this case there was a potential scheduling point in the code, that meant
that I couldn't change kmap to kmap_atomic.

So the points I was making above were that kmap isn't required around
sendpage, because sendpage will happily take unmapped pages. You can see
that by looking at sock_no_sendpage() which emulates sendpage for those
protocols which don't define their own method. Also, that in the other
cases, the reason that we've never run into any problems with GFP_KERNEL
allocations occurring is that the kmaps in question have never been
supplied with pages in highmem.

It is left as an exercise for the reader to consider whether its a bug
that DLM isn't using highmem pages for its internal buffers (in which
case we'd have to solve the allocation problem at kmap time), or whether
its a bug that the kmap/kunmap pairs are there at all (and can thus be
removed, which is the simpler solution) :-)

Steve.

[Cluster-devel] Re: [DLM] Fix up memory alloc/kmap

[David Teigland]

On Thu, Nov 13, 2008 at 09:56:18AM +0000, Steven Whitehouse wrote:
> It is left as an exercise for the reader to consider whether its a bug
> that DLM isn't using highmem pages for its internal buffers (in which
> case we'd have to solve the allocation problem at kmap time), or whether
> its a bug that the kmap/kunmap pairs are there at all (and can thus be
> removed, which is the simpler solution) :-)

Thanks, I'll remove all the kmap/kunmap calls, then.