From 2b6e72ed747f68a038df616efd86744b3644d694 Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Fri, 17 Jan 2014 18:31:00 +0100 Subject: 9P: Add memory barriers to protect request fields over cb/rpc threads handoff We need barriers to guarantee this pattern works as intended: [w] req->rc, 1 [r] req->status, 1 wmb rmb [w] req->status, 1 [r] req->rc Where the wmb ensures that rc gets written before status, and the rmb ensures that if you observe status == 1, rc is the new value. Signed-off-by: Dominique Martinet Signed-off-by: Eric Van Hensbergen --- net/9p/client.c | 16 +++++++++++++++- net/9p/trans_fd.c | 15 ++++++--------- net/9p/trans_rdma.c | 3 +-- net/9p/trans_virtio.c | 3 +-- 4 files changed, 23 insertions(+), 14 deletions(-) (limited to 'net/9p') diff --git a/net/9p/client.c b/net/9p/client.c index a5e4d2dcb03e..95b1836173e1 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -415,9 +415,17 @@ static void p9_free_req(struct p9_client *c, struct p9_req_t *r) * req: request received * */ -void p9_client_cb(struct p9_client *c, struct p9_req_t *req) +void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status) { p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc->tag); + + /* + * This barrier is needed to make sure any change made to req before + * the other thread wakes up will indeed be seen by the waiting side. + */ + smp_wmb(); + req->status = status; + wake_up(req->wq); p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag); } @@ -751,6 +759,12 @@ again: err = wait_event_interruptible(*req->wq, req->status >= REQ_STATUS_RCVD); + /* + * Make sure our req is coherent with regard to updates in other + * threads - echoes to wmb() in the callback + */ + smp_rmb(); + if ((err == -ERESTARTSYS) && (c->status == Connected) && (type == P9_TFLUSH)) { sigpending = 1; diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index b7bd7f2961bf..193efd562466 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -212,15 +212,9 @@ static void p9_conn_cancel(struct p9_conn *m, int err) m->err = err; list_for_each_entry_safe(req, rtmp, &m->req_list, req_list) { - req->status = REQ_STATUS_ERROR; - if (!req->t_err) - req->t_err = err; list_move(&req->req_list, &cancel_list); } list_for_each_entry_safe(req, rtmp, &m->unsent_req_list, req_list) { - req->status = REQ_STATUS_ERROR; - if (!req->t_err) - req->t_err = err; list_move(&req->req_list, &cancel_list); } spin_unlock_irqrestore(&m->client->lock, flags); @@ -228,7 +222,9 @@ static void p9_conn_cancel(struct p9_conn *m, int err) list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) { p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req); list_del(&req->req_list); - p9_client_cb(m->client, req); + if (!req->t_err) + req->t_err = err; + p9_client_cb(m->client, req, REQ_STATUS_ERROR); } } @@ -302,6 +298,7 @@ static void p9_read_work(struct work_struct *work) { int n, err; struct p9_conn *m; + int status = REQ_STATUS_ERROR; m = container_of(work, struct p9_conn, rq); @@ -375,10 +372,10 @@ static void p9_read_work(struct work_struct *work) p9_debug(P9_DEBUG_TRANS, "got new packet\n"); spin_lock(&m->client->lock); if (m->req->status != REQ_STATUS_ERROR) - m->req->status = REQ_STATUS_RCVD; + status = REQ_STATUS_RCVD; list_del(&m->req->req_list); spin_unlock(&m->client->lock); - p9_client_cb(m->client, m->req); + p9_client_cb(m->client, m->req, status); m->rbuf = NULL; m->rpos = 0; m->rsize = 0; diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index 8f68df5d2973..f127ae504911 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -305,8 +305,7 @@ handle_recv(struct p9_client *client, struct p9_trans_rdma *rdma, } req->rc = c->rc; - req->status = REQ_STATUS_RCVD; - p9_client_cb(client, req); + p9_client_cb(client, req, REQ_STATUS_RCVD); return; diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index cd1e1ede73a4..a8c67e2be3f8 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -164,8 +164,7 @@ static void req_done(struct virtqueue *vq) p9_debug(P9_DEBUG_TRANS, ": rc %p\n", rc); p9_debug(P9_DEBUG_TRANS, ": lookup tag %d\n", rc->tag); req = p9_tag_lookup(chan->client, rc->tag); - req->status = REQ_STATUS_RCVD; - p9_client_cb(chan->client, req); + p9_client_cb(chan->client, req, REQ_STATUS_RCVD); } } -- cgit From 05a782d416944593ca0268d5718fe4b9cba5ef67 Mon Sep 17 00:00:00 2001 From: Rashika Date: Sun, 9 Feb 2014 19:57:33 +0530 Subject: net: Mark function as static in 9p/client.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark function as static in net/9p/client.c because it is not used outside this file. This eliminates the following warning in net/9p/client.c: net/9p/client.c:207:18: warning: no previous prototype for ‘p9_fcall_alloc’ [-Wmissing-prototypes] Signed-off-by: Rashika Kheria Reviewed-by: Josh Triplett Signed-off-by: Eric Van Hensbergen --- net/9p/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/9p') diff --git a/net/9p/client.c b/net/9p/client.c index 95b1836173e1..ce26da95f63f 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -204,7 +204,7 @@ free_and_return: return ret; } -struct p9_fcall *p9_fcall_alloc(int alloc_msize) +static struct p9_fcall *p9_fcall_alloc(int alloc_msize) { struct p9_fcall *fc; fc = kmalloc(sizeof(struct p9_fcall) + alloc_msize, GFP_NOFS); -- cgit From afd8d65411551839b7ab14a539d00075b2793451 Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Mon, 10 Mar 2014 16:38:49 +0100 Subject: 9P: Add cancelled() to the transport functions. And move transport-specific code out of net/9p/client.c Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/client.c | 9 +++------ net/9p/trans_fd.c | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) (limited to 'net/9p') diff --git a/net/9p/client.c b/net/9p/client.c index ce26da95f63f..40e558172bbe 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -663,16 +663,13 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq) if (IS_ERR(req)) return PTR_ERR(req); - /* * if we haven't received a response for oldreq, * remove it from the list */ - if (oldreq->status == REQ_STATUS_FLSH) { - spin_lock(&c->lock); - list_del(&oldreq->req_list); - spin_unlock(&c->lock); - } + if (oldreq->status == REQ_STATUS_FLSH) + if (c->trans_mod->cancelled) + c->trans_mod->cancelled(c, oldreq); p9_free_req(c, req); return 0; diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 193efd562466..fda4951c869e 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -709,6 +709,20 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) return ret; } +static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req) +{ + p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); + + /* we haven't received a response for oldreq, + * remove it from the list. + */ + spin_lock(&client->lock); + list_del(&req->req_list); + spin_unlock(&client->lock); + + return 0; +} + /** * parse_opts - parse mount options into p9_fd_opts structure * @params: options string passed from mount @@ -1050,6 +1064,7 @@ static struct p9_trans_module p9_tcp_trans = { .close = p9_fd_close, .request = p9_fd_request, .cancel = p9_fd_cancel, + .cancelled = p9_fd_cancelled, .owner = THIS_MODULE, }; @@ -1061,6 +1076,7 @@ static struct p9_trans_module p9_unix_trans = { .close = p9_fd_close, .request = p9_fd_request, .cancel = p9_fd_cancel, + .cancelled = p9_fd_cancelled, .owner = THIS_MODULE, }; @@ -1072,6 +1088,7 @@ static struct p9_trans_module p9_fd_trans = { .close = p9_fd_close, .request = p9_fd_request, .cancel = p9_fd_cancel, + .cancelled = p9_fd_cancelled, .owner = THIS_MODULE, }; -- cgit From 3f9d5b8dfdc7b013287caaa847b378912b013f77 Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Mon, 10 Mar 2014 16:38:50 +0100 Subject: 9pnet_rdma: update request status during send This will be needed by the flush logic. Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/trans_rdma.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'net/9p') diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index f127ae504911..b374c578ddb8 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -510,6 +510,11 @@ dont_need_post_recv: goto send_error; } + /* Mark request as `sent' *before* we actually send it, + * because doing if after could erase the REQ_STATUS_RCVD + * status in case of a very fast reply. + */ + req->status = REQ_STATUS_SENT; err = ib_post_send(rdma->qp, &wr, &bad_wr); if (err) goto send_error; @@ -519,6 +524,7 @@ dont_need_post_recv: /* Handle errors that happened during or while preparing the send: */ send_error: + req->status = REQ_STATUS_ERROR; kfree(c); p9_debug(P9_DEBUG_ERROR, "Error %d in rdma_request()\n", err); -- cgit From 931700d26bf17c583805f52121a41231e64868f3 Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Mon, 10 Mar 2014 16:38:51 +0100 Subject: 9pnet_rdma: add cancelled() Take into account posted recv buffers that will never receive their reply. The RDMA code posts a recv buffer for each request that it sends. When a request is flushed, it is possible that this request will never receive a reply, and that one recv buffer will stay unused on the recv queue. It is then possible, if this scenario happens several times, to have the recv queue full, and have the 9pnet_rmda module unable to send new requests. Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/trans_rdma.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) (limited to 'net/9p') diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index b374c578ddb8..8f5e4f769d13 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -587,12 +587,24 @@ static struct p9_trans_rdma *alloc_rdma(struct p9_rdma_opts *opts) return rdma; } -/* its not clear to me we can do anything after send has been posted */ static int rdma_cancel(struct p9_client *client, struct p9_req_t *req) { + /* Nothing to do here. + * We will take care of it (if we have to) in rdma_cancelled() + */ return 1; } +/* A request has been fully flushed without a reply. + * That means we have posted one buffer in excess. + */ +static int rdma_cancelled(struct p9_client *client, struct p9_req_t *req) +{ + struct p9_trans_rdma *rdma = client->trans; + atomic_inc(&rdma->excess_rc); + return 0; +} + /** * trans_create_rdma - Transport method for creating atransport instance * @client: client instance @@ -726,6 +738,7 @@ static struct p9_trans_module p9_rdma_trans = { .close = rdma_close, .request = rdma_request, .cancel = rdma_cancel, + .cancelled = rdma_cancelled, }; /** -- cgit From 0bfd6845c0b5e3df3507d2b4d230478980e7f04a Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Mon, 10 Mar 2014 16:38:52 +0100 Subject: 9P: Get rid of REQ_STATUS_FLSH This request state is mostly useless, and properly implementing it for RDMA would require an extra lock to be taken in handle_recv() and in rdma_cancel() to avoid this race: handle_recv() rdma_cancel() . . . if req->state == SENT req->state = RCVD . . req->state = FLSH So just get rid of it. Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/client.c | 2 +- net/9p/trans_fd.c | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) (limited to 'net/9p') diff --git a/net/9p/client.c b/net/9p/client.c index 40e558172bbe..0004cbaac4a4 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -667,7 +667,7 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq) * if we haven't received a response for oldreq, * remove it from the list */ - if (oldreq->status == REQ_STATUS_FLSH) + if (oldreq->status == REQ_STATUS_SENT) if (c->trans_mod->cancelled) c->trans_mod->cancelled(c, oldreq); diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index fda4951c869e..9c806428e91a 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -345,8 +345,7 @@ static void p9_read_work(struct work_struct *work) "mux %p pkt: size: %d bytes tag: %d\n", m, n, tag); m->req = p9_tag_lookup(m->client, tag); - if (!m->req || (m->req->status != REQ_STATUS_SENT && - m->req->status != REQ_STATUS_FLSH)) { + if (!m->req || (m->req->status != REQ_STATUS_SENT)) { p9_debug(P9_DEBUG_ERROR, "Unexpected packet tag %d\n", tag); err = -EIO; @@ -701,9 +700,7 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req) list_del(&req->req_list); req->status = REQ_STATUS_FLSHD; ret = 0; - } else if (req->status == REQ_STATUS_SENT) - req->status = REQ_STATUS_FLSH; - + } spin_unlock(&client->lock); return ret; -- cgit From 263c582888d3cd2a5c8e32225140723ec31f1709 Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Tue, 11 Mar 2014 10:13:09 +0100 Subject: 9pnet: trans_fd : allocate struct p9_trans_fd and struct p9_conn together. There is no point in allocating these structs separately. Changing this makes the code a little simpler and saves a few bytes of memory. Reported-by: Herve Vico Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/trans_fd.c | 71 ++++++++++++++++++++----------------------------------- 1 file changed, 25 insertions(+), 46 deletions(-) (limited to 'net/9p') diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 9c806428e91a..80d08f6664cb 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -66,20 +66,6 @@ struct p9_fd_opts { int privport; }; -/** - * struct p9_trans_fd - transport state - * @rd: reference to file to read from - * @wr: reference of file to write to - * @conn: connection state reference - * - */ - -struct p9_trans_fd { - struct file *rd; - struct file *wr; - struct p9_conn *conn; -}; - /* * Option Parsing (code inspired by NFS code) * - a little lazy - parse all fd-transport options @@ -159,6 +145,20 @@ struct p9_conn { unsigned long wsched; }; +/** + * struct p9_trans_fd - transport state + * @rd: reference to file to read from + * @wr: reference of file to write to + * @conn: connection state reference + * + */ + +struct p9_trans_fd { + struct file *rd; + struct file *wr; + struct p9_conn conn; +}; + static void p9_poll_workfn(struct work_struct *work); static DEFINE_SPINLOCK(p9_poll_lock); @@ -569,21 +569,19 @@ p9_pollwait(struct file *filp, wait_queue_head_t *wait_address, poll_table *p) } /** - * p9_conn_create - allocate and initialize the per-session mux data + * p9_conn_create - initialize the per-session mux data * @client: client instance * * Note: Creates the polling task if this is the first session. */ -static struct p9_conn *p9_conn_create(struct p9_client *client) +static void p9_conn_create(struct p9_client *client) { int n; - struct p9_conn *m; + struct p9_trans_fd *ts = client->trans; + struct p9_conn *m = &ts->conn; p9_debug(P9_DEBUG_TRANS, "client %p msize %d\n", client, client->msize); - m = kzalloc(sizeof(struct p9_conn), GFP_KERNEL); - if (!m) - return ERR_PTR(-ENOMEM); INIT_LIST_HEAD(&m->mux_list); m->client = client; @@ -605,8 +603,6 @@ static struct p9_conn *p9_conn_create(struct p9_client *client) p9_debug(P9_DEBUG_TRANS, "mux %p can write\n", m); set_bit(Wpending, &m->wsched); } - - return m; } /** @@ -665,7 +661,7 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req) { int n; struct p9_trans_fd *ts = client->trans; - struct p9_conn *m = ts->conn; + struct p9_conn *m = &ts->conn; p9_debug(P9_DEBUG_TRANS, "mux %p task %p tcall %p id %d\n", m, current, req->tc, req->tc->id); @@ -788,7 +784,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) static int p9_fd_open(struct p9_client *client, int rfd, int wfd) { - struct p9_trans_fd *ts = kmalloc(sizeof(struct p9_trans_fd), + struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), GFP_KERNEL); if (!ts) return -ENOMEM; @@ -814,9 +810,8 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) { struct p9_trans_fd *p; struct file *file; - int ret; - p = kmalloc(sizeof(struct p9_trans_fd), GFP_KERNEL); + p = kzalloc(sizeof(struct p9_trans_fd), GFP_KERNEL); if (!p) return -ENOMEM; @@ -837,20 +832,12 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) p->rd->f_flags |= O_NONBLOCK; - p->conn = p9_conn_create(client); - if (IS_ERR(p->conn)) { - ret = PTR_ERR(p->conn); - p->conn = NULL; - kfree(p); - sockfd_put(csocket); - sockfd_put(csocket); - return ret; - } + p9_conn_create(client); return 0; } /** - * p9_mux_destroy - cancels all pending requests and frees mux resources + * p9_mux_destroy - cancels all pending requests of mux * @m: mux to destroy * */ @@ -867,7 +854,6 @@ static void p9_conn_destroy(struct p9_conn *m) p9_conn_cancel(m, -ECONNRESET); m->client = NULL; - kfree(m); } /** @@ -889,7 +875,7 @@ static void p9_fd_close(struct p9_client *client) client->status = Disconnected; - p9_conn_destroy(ts->conn); + p9_conn_destroy(&ts->conn); if (ts->rd) fput(ts->rd); @@ -1041,14 +1027,7 @@ p9_fd_create(struct p9_client *client, const char *addr, char *args) return err; p = (struct p9_trans_fd *) client->trans; - p->conn = p9_conn_create(client); - if (IS_ERR(p->conn)) { - err = PTR_ERR(p->conn); - p->conn = NULL; - fput(p->rd); - fput(p->wr); - return err; - } + p9_conn_create(client); return 0; } -- cgit From 8a5daf1e2c199746767d94b4036638ba11d528b3 Mon Sep 17 00:00:00 2001 From: Simon Derr Date: Tue, 11 Mar 2014 10:14:15 +0100 Subject: 9pnet_rdma: check token type before int conversion When parsing options, make sure we have found a proper token before doing a numeric conversion. Without this check, the current code will end up following random pointers that just happened to be on the stack when this function was called, because match_token() will not touch the 'args' list unless a valid token is found. Signed-off-by: Simon Derr Signed-off-by: Eric Van Hensbergen --- net/9p/trans_rdma.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'net/9p') diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index 8f5e4f769d13..14ad43b5cf89 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -193,6 +193,8 @@ static int parse_opts(char *params, struct p9_rdma_opts *opts) if (!*p) continue; token = match_token(p, tokens, args); + if (token == Opt_err) + continue; r = match_int(&args[0], &option); if (r < 0) { p9_debug(P9_DEBUG_ERROR, -- cgit