Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1476030 > unrolled thread

[PATCH net-next 0/9] rxrpc: Small fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/9] rxrpc: Small fixes David Howells <dhowells@redhat.com> - 2016-09-04 23:10 +0200
    [PATCH net-next 9/9] fs/afs/flock: Remove deprecated  create_singlethread_workqueue David Howells <dhowells@redhat.com> - 2016-09-04 23:10 +0200
    [PATCH net-next 5/9] rxrpc: Don't change the epoch David Howells <dhowells@redhat.com> - 2016-09-04 23:10 +0200
    [PATCH net-next 6/9] fs/afs/vlocation: Remove deprecated  create_singlethread_workqueue David Howells <dhowells@redhat.com> - 2016-09-04 23:20 +0200
    [PATCH net-next 3/9] rxrpc: The client call state must be changed  before attachment to conn David Howells <dhowells@redhat.com> - 2016-09-04 23:20 +0200
    [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client  conn ID values David Howells <dhowells@redhat.com> - 2016-09-04 23:20 +0200
      RE: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client  conn ID values David Laight <David.Laight@ACULAB.COM> - 2016-09-05 17:10 +0200
        Re: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client conn ID values David Howells <dhowells@redhat.com> - 2016-09-05 18:30 +0200
          Re: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client  conn ID values Jeffrey Altman <jaltman@auristor.com> - 2016-09-06 04:20 +0200
    [PATCH net-next 7/9] fs/afs/rxrpc: Remove deprecated  create_singlethread_workqueue David Howells <dhowells@redhat.com> - 2016-09-04 23:20 +0200
    [PATCH net-next 8/9] fs/afs/callback: Remove deprecated  create_singlethread_workqueue David Howells <dhowells@redhat.com> - 2016-09-04 23:20 +0200
    Re: [PATCH net-next 0/9] rxrpc: Small fixes David Miller <davem@davemloft.net> - 2016-09-06 23:00 +0200

#1476030 — [PATCH net-next 0/9] rxrpc: Small fixes

Here's a set of small fix patches:

 (1) Fix some uninitialised variables.

 (2) Set the client call state before making it live by attaching it to the
     conn struct.

 (3) Randomise the epoch and starting client conn ID values, and don't
     change the epoch when the client conn ID rolls round.

 (4) Replace deprecated create_singlethread_workqueue() calls.

The patches can be found here also (non-terminally on the branch):

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rewrite

Tagged thusly:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	rxrpc-rewrite-20160904-1

David
---
Arnd Bergmann (1):
      rxrpc: fix undefined behavior in rxrpc_mark_call_released

Bhaktipriya Shridhar (4):
      fs/afs/vlocation: Remove deprecated create_singlethread_workqueue
      fs/afs/rxrpc: Remove deprecated create_singlethread_workqueue
      fs/afs/callback: Remove deprecated create_singlethread_workqueue
      fs/afs/flock: Remove deprecated create_singlethread_workqueue

David Howells (4):
      rxrpc: Fix uninitialised variable warning
      rxrpc: The client call state must be changed before attachment to conn
      rxrpc: Randomise epoch and starting client conn ID values
      rxrpc: Don't change the epoch


 fs/afs/callback.c       |    4 ++--
 fs/afs/flock.c          |    4 ++--
 fs/afs/rxrpc.c          |    2 +-
 fs/afs/vlocation.c      |    4 ++--
 include/rxrpc/packet.h  |    1 +
 net/rxrpc/af_rxrpc.c    |    9 ++++++++-
 net/rxrpc/call_event.c  |    5 ++---
 net/rxrpc/call_object.c |    4 +---
 net/rxrpc/conn_client.c |   36 ++++++++++++------------------------
 9 files changed, 31 insertions(+), 38 deletions(-)

[toc] | [next] | [standalone]

#1476032 — [PATCH net-next 9/9] fs/afs/flock: Remove deprecated create_singlethread_workqueue

From: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

The workqueue "afs_lock_manager" queues work item &vnode->lock_work,
per vnode. Since there can be multiple vnodes and since their work items
can be executed concurrently, alloc_workqueue has been used to replace
the deprecated create_singlethread_workqueue instance.

The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
memory pressure because the workqueue is being used on a memory reclaim
path.

Since there are fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/flock.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/flock.c b/fs/afs/flock.c
index d91a9c9cfbd0..3191dff2c156 100644
--- a/fs/afs/flock.c
+++ b/fs/afs/flock.c
@@ -36,8 +36,8 @@ static int afs_init_lock_manager(void)
 	if (!afs_lock_manager) {
 		mutex_lock(&afs_lock_manager_mutex);
 		if (!afs_lock_manager) {
-			afs_lock_manager =
-				create_singlethread_workqueue("kafs_lockd");
+			afs_lock_manager = alloc_workqueue("kafs_lockd",
+							   WQ_MEM_RECLAIM, 0);
 			if (!afs_lock_manager)
 				ret = -ENOMEM;
 		}

[toc] | [prev] | [next] | [standalone]

#1476033 — [PATCH net-next 5/9] rxrpc: Don't change the epoch

It seems the local epoch should only be changed on boot, so remove the code
that changes it for client connections.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/conn_client.c |   32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index e19804dd6c8d..82de1aeaef21 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -108,12 +108,12 @@ static DECLARE_DELAYED_WORK(rxrpc_client_conn_reap,
 /*
  * Get a connection ID and epoch for a client connection from the global pool.
  * The connection struct pointer is then recorded in the idr radix tree.  The
- * epoch is changed if this wraps.
+ * epoch doesn't change until the client is rebooted (or, at least, unless the
+ * module is unloaded).
  */
 static int rxrpc_get_client_connection_id(struct rxrpc_connection *conn,
 					  gfp_t gfp)
 {
-	u32 epoch;
 	int id;
 
 	_enter("");
@@ -121,34 +121,18 @@ static int rxrpc_get_client_connection_id(struct rxrpc_connection *conn,
 	idr_preload(gfp);
 	spin_lock(&rxrpc_conn_id_lock);
 
-	epoch = rxrpc_epoch;
-
-	/* We could use idr_alloc_cyclic() here, but we really need to know
-	 * when the thing wraps so that we can advance the epoch.
-	 */
-	if (rxrpc_client_conn_ids.cur == 0)
-		rxrpc_client_conn_ids.cur = 1;
-	id = idr_alloc(&rxrpc_client_conn_ids, conn,
-		       rxrpc_client_conn_ids.cur, 0x40000000, GFP_NOWAIT);
-	if (id < 0) {
-		if (id != -ENOSPC)
-			goto error;
-		id = idr_alloc(&rxrpc_client_conn_ids, conn,
-			       1, 0x40000000, GFP_NOWAIT);
-		if (id < 0)
-			goto error;
-		epoch++;
-		rxrpc_epoch = epoch;
-	}
-	rxrpc_client_conn_ids.cur = id + 1;
+	id = idr_alloc_cyclic(&rxrpc_client_conn_ids, conn,
+			      1, 0x40000000, GFP_NOWAIT);
+	if (id < 0)
+		goto error;
 
 	spin_unlock(&rxrpc_conn_id_lock);
 	idr_preload_end();
 
-	conn->proto.epoch = epoch;
+	conn->proto.epoch = rxrpc_epoch;
 	conn->proto.cid = id << RXRPC_CIDSHIFT;
 	set_bit(RXRPC_CONN_HAS_IDR, &conn->flags);
-	_leave(" [CID %x:%x]", epoch, conn->proto.cid);
+	_leave(" [CID %x]", conn->proto.cid);
 	return 0;
 
 error:

[toc] | [prev] | [next] | [standalone]

#1476035 — [PATCH net-next 6/9] fs/afs/vlocation: Remove deprecated create_singlethread_workqueue

From: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

The workqueue "afs_vlocation_update_worker" queues a single work item
&afs_vlocation_update and hence it doesn't require execution ordering.
Hence, alloc_workqueue has been used to replace the deprecated
create_singlethread_workqueue instance.

Since the workqueue is being used on a memory reclaim path, WQ_MEM_RECLAIM
flag has been set to ensure forward progress under memory pressure.

Since there are fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/vlocation.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/vlocation.c b/fs/afs/vlocation.c
index 52976785a32c..45a86396fd2d 100644
--- a/fs/afs/vlocation.c
+++ b/fs/afs/vlocation.c
@@ -594,8 +594,8 @@ static void afs_vlocation_reaper(struct work_struct *work)
  */
 int __init afs_vlocation_update_init(void)
 {
-	afs_vlocation_update_worker =
-		create_singlethread_workqueue("kafs_vlupdated");
+	afs_vlocation_update_worker = alloc_workqueue("kafs_vlupdated",
+						      WQ_MEM_RECLAIM, 0);
 	return afs_vlocation_update_worker ? 0 : -ENOMEM;
 }
 

[toc] | [prev] | [next] | [standalone]

#1476036 — [PATCH net-next 3/9] rxrpc: The client call state must be changed before attachment to conn

We must set the client call state to RXRPC_CALL_CLIENT_SEND_REQUEST before
attaching the call to the connection struct, not after, as it's liable to
receive errors and conn aborts as soon as the assignment is made - and
these will cause its state to be changed outside of the initiating thread's
control.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/call_object.c |    2 --
 net/rxrpc/conn_client.c |    4 ++++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 57e00fc9cff2..65691742199b 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -197,8 +197,6 @@ static int rxrpc_begin_client_call(struct rxrpc_call *call,
 	if (ret < 0)
 		return ret;
 
-	call->state = RXRPC_CALL_CLIENT_SEND_REQUEST;
-
 	spin_lock(&call->conn->params.peer->lock);
 	hlist_add_head(&call->error_link, &call->conn->params.peer->error_targets);
 	spin_unlock(&call->conn->params.peer->lock);
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 4b213bc0f554..e19804dd6c8d 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -537,6 +537,10 @@ static void rxrpc_activate_one_channel(struct rxrpc_connection *conn,
 					     struct rxrpc_call, chan_wait_link);
 	u32 call_id = chan->call_counter + 1;
 
+	write_lock_bh(&call->state_lock);
+	call->state = RXRPC_CALL_CLIENT_SEND_REQUEST;
+	write_unlock_bh(&call->state_lock);
+
 	rxrpc_see_call(call);
 	list_del_init(&call->chan_wait_link);
 	conn->active_chans |= 1 << channel;

[toc] | [prev] | [next] | [standalone]

#1476038 — [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client conn ID values

Create a random epoch value rather than a time-based one on startup and set
the top bit to indicate that this is the case.

Also create a random starting client connection ID value.  This will be
incremented from here as new client connections are created.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/rxrpc/packet.h |    1 +
 net/rxrpc/af_rxrpc.c   |    9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/rxrpc/packet.h b/include/rxrpc/packet.h
index b2017440b765..3c6128e1fdbe 100644
--- a/include/rxrpc/packet.h
+++ b/include/rxrpc/packet.h
@@ -24,6 +24,7 @@ typedef __be32	rxrpc_serial_net_t; /* on-the-wire Rx message serial number */
  */
 struct rxrpc_wire_header {
 	__be32		epoch;		/* client boot timestamp */
+#define RXRPC_RANDOM_EPOCH	0x80000000	/* Random if set, date-based if not */
 
 	__be32		cid;		/* connection and channel ID */
 #define RXRPC_MAXCALLS		4			/* max active calls per conn */
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 32d544995dda..b66a9e6f8d04 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -16,6 +16,7 @@
 #include <linux/net.h>
 #include <linux/slab.h>
 #include <linux/skbuff.h>
+#include <linux/random.h>
 #include <linux/poll.h>
 #include <linux/proc_fs.h>
 #include <linux/key-type.h>
@@ -700,7 +701,13 @@ static int __init af_rxrpc_init(void)
 
 	BUILD_BUG_ON(sizeof(struct rxrpc_skb_priv) > FIELD_SIZEOF(struct sk_buff, cb));
 
-	rxrpc_epoch = get_seconds();
+	get_random_bytes(&rxrpc_epoch, sizeof(rxrpc_epoch));
+	rxrpc_epoch |= RXRPC_RANDOM_EPOCH;
+	get_random_bytes(&rxrpc_client_conn_ids.cur,
+			 sizeof(rxrpc_client_conn_ids.cur));
+	rxrpc_client_conn_ids.cur &= 0x3fffffff;
+	if (rxrpc_client_conn_ids.cur == 0)
+		rxrpc_client_conn_ids.cur = 1;
 
 	ret = -ENOMEM;
 	rxrpc_call_jar = kmem_cache_create(

[toc] | [prev] | [next] | [standalone]

#1476539 — RE: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client conn ID values

From: David Howells
> Sent: 04 September 2016 22:03
> Create a random epoch value rather than a time-based one on startup and set
> the top bit to indicate that this is the case.

Why set the top bit?
There is nothing to stop the time (in seconds) from having the top bit set.
Nothing else can care - otherwise this wouldn't work.

> Also create a random starting client connection ID value.  This will be
> incremented from here as new client connections are created.

I'm guessing this is to make duplicates less likely after a restart?
You may want to worry about duplicate allocations (after 2^32 connects).
There are id allocation algorithms that guarantee not to generate duplicates
and not to reuse values quickly while still being fixed cost.
Look at the code NetBSD uses to allocate process ids for an example.

	David

[toc] | [prev] | [next] | [standalone]

#1476586 — Re: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client conn ID values

[cc'ing Jeff Altman for comment]

David Laight <David.Laight@ACULAB.COM> wrote:

> > Create a random epoch value rather than a time-based one on startup and set
> > the top bit to indicate that this is the case.
> 
> Why set the top bit?
> There is nothing to stop the time (in seconds) from having the top bit set.
> Nothing else can care - otherwise this wouldn't work.

This is what I'm told I should do by purveyors of other RxRPC solutions.

> > Also create a random starting client connection ID value.  This will be
> > incremented from here as new client connections are created.
> 
> I'm guessing this is to make duplicates less likely after a restart?

Again, it's been suggested that I do this, but I would guess so.

> You may want to worry about duplicate allocations (after 2^32 connects).

It's actually a quarter of that, but connection != call, so a connection may
be used for up to ~16 billion RPC operations before it *has* to be flushed.

> There are id allocation algorithms that guarantee not to generate duplicates
> and not to reuse values quickly while still being fixed cost.
> Look at the code NetBSD uses to allocate process ids for an example.

I'm using idr_alloc_cyclic()[*] with a fixed size "window" on the active conn
ID values.  Client connections with IDs outside of that window are discarded
as soon as possible to keep the memory consumption of the tree down (and to
force security renegotiation occasionally).  However, given that there are a
billion IDs to cycle through, it will take quite a while for reuse to become
an issue.

I like the idea of incrementing the epoch every time we cycle through the ID
space, but I'm told that a change in the epoch value is an indication that the
client rebooted - with what consequences I cannot say.

[*] which is what Linux uses to allocate process IDs.

David

[toc] | [prev] | [next] | [standalone]

#1477020 — Re: [PATCH net-next 4/9] rxrpc: Randomise epoch and starting client conn ID values

[Multipart message — attachments visible in raw view] — view raw

Reply inline ....

On 9/5/2016 12:24 PM, David Howells wrote:
> [cc'ing Jeff Altman for comment]
> 
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
>>> Create a random epoch value rather than a time-based one on startup and set
>>> the top bit to indicate that this is the case.
>>
>> Why set the top bit?
>> There is nothing to stop the time (in seconds) from having the top bit set.
>> Nothing else can care - otherwise this wouldn't work.
> 
> This is what I'm told I should do by purveyors of other RxRPC solutions.

The protocol specification requires that the top bit be 1 for a random
epoch and 0 for a time derived epoch.
> 
>>> Also create a random starting client connection ID value.  This will be
>>> incremented from here as new client connections are created.
>>
>> I'm guessing this is to make duplicates less likely after a restart?

Its to reduce the possibility of duplicates on multiple machines that
might at some point exchange an endpoint address either due to mobility
or NAT/PAT.
> 
> Again, it's been suggested that I do this, but I would guess so.
> 
>> You may want to worry about duplicate allocations (after 2^32 connects).
> 
> It's actually a quarter of that, but connection != call, so a connection may
> be used for up to ~16 billion RPC operations before it *has* to be flushed.
> 
>> There are id allocation algorithms that guarantee not to generate duplicates
>> and not to reuse values quickly while still being fixed cost.
>> Look at the code NetBSD uses to allocate process ids for an example.
> 
> I'm using idr_alloc_cyclic()[*] with a fixed size "window" on the active conn
> ID values.  Client connections with IDs outside of that window are discarded
> as soon as possible to keep the memory consumption of the tree down (and to
> force security renegotiation occasionally).  However, given that there are a
> billion IDs to cycle through, it will take quite a while for reuse to become
> an issue.
> 
> I like the idea of incrementing the epoch every time we cycle through the ID
> space, but I'm told that a change in the epoch value is an indication that the
> client rebooted - with what consequences I cannot say.

State information might be recorded about an rx peer with the assumption
that state will be reset when the epoch changes.  The most frequent use
of this technique is for rx rpc statistics monitoring.


> 
> [*] which is what Linux uses to allocate process IDs.
> 
> David
> 

[toc] | [prev] | [next] | [standalone]

#1476039 — [PATCH net-next 7/9] fs/afs/rxrpc: Remove deprecated create_singlethread_workqueue

From: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

The workqueue "afs_async_calls" queues work item
&call->async_work per afs_call. Since there could be multiple calls and since
these calls can be run concurrently, alloc_workqueue has been used to replace
the deprecated create_singlethread_workqueue instance.

The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
memory pressure because the workqueue is being used on a memory reclaim
path.

Since there are fixed number of work items, explicit concurrency
limit is unnecessary here.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/rxrpc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index 244896baf241..37608be52abd 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -76,7 +76,7 @@ int afs_open_socket(void)
 	_enter("");
 
 	ret = -ENOMEM;
-	afs_async_calls = create_singlethread_workqueue("kafsd");
+	afs_async_calls = alloc_workqueue("kafsd", WQ_MEM_RECLAIM, 0);
 	if (!afs_async_calls)
 		goto error_0;
 

[toc] | [prev] | [next] | [standalone]

#1476040 — [PATCH net-next 8/9] fs/afs/callback: Remove deprecated create_singlethread_workqueue

From: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

The workqueue "afs_callback_update_worker" queues multiple work items
viz  &vnode->cb_broken_work, &server->cb_break_work which require strict
execution ordering. Hence, an ordered dedicated workqueue has been used.

Since the workqueue is being used on a memory reclaim path, WQ_MEM_RECLAIM
has been set to ensure forward progress under memory pressure.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/callback.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/callback.c b/fs/afs/callback.c
index 7ef637d7f3a5..1e9d2f84e5b5 100644
--- a/fs/afs/callback.c
+++ b/fs/afs/callback.c
@@ -461,8 +461,8 @@ static void afs_callback_updater(struct work_struct *work)
  */
 int __init afs_callback_update_init(void)
 {
-	afs_callback_update_worker =
-		create_singlethread_workqueue("kafs_callbackd");
+	afs_callback_update_worker = alloc_ordered_workqueue("kafs_callbackd",
+							     WQ_MEM_RECLAIM);
 	return afs_callback_update_worker ? 0 : -ENOMEM;
 }
 

[toc] | [prev] | [next] | [standalone]

#1477820

From: David Howells <dhowells@redhat.com>
Date: Sun, 04 Sep 2016 22:02:24 +0100

> 
> Here's a set of small fix patches:
> 
>  (1) Fix some uninitialised variables.
> 
>  (2) Set the client call state before making it live by attaching it to the
>      conn struct.
> 
>  (3) Randomise the epoch and starting client conn ID values, and don't
>      change the epoch when the client conn ID rolls round.
> 
>  (4) Replace deprecated create_singlethread_workqueue() calls.
> 
> The patches can be found here also (non-terminally on the branch):
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-rewrite
> 
> Tagged thusly:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
> 	rxrpc-rewrite-20160904-1

Both rxrpc-rewrite-20160904-1 and rxrpc-rewrite-20160904-2 pulled, thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web