On Wed, 11 Jul 2018 14:02:23 -0700
Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> On a 64-bit system, the wait_queue_head_t is 24 bytes while the pointer
> to it is 8 bytes. Growing the p9_req_t by 16 bytes is better than
> performing a 24-byte memory allocation.
>
This is true when all tags have been used at least once. But the current code
lazily allocates the wait_queue_head_t, ie, only when a tag is used for the
first time. Your patch causes a full row of wait_quest_head_t to be pre-allocated.
ie, P9_ROW_MAXTAG * 24 = 255 * 24 = 6120
instead of (P9_ROW_MAXTAG * 8) + 24 = 255 * 8 + 24 = 2064
This is nearly a page of allocated memory that might be never used.
Not sure if this is a problem though...
> Signed-off-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> ---
> include/net/9p/client.h | 2 +-
> net/9p/client.c | 19 +++++--------------
> net/9p/trans_virtio.c | 2 +-
> 3 files changed, 7 insertions(+), 16 deletions(-)
>
... and the diffstat is nice :) so
Reviewed-by: Greg Kurz <groug@xxxxxxxx>
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index e405729cd1c7..0fa0fbab33b0 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -113,7 +113,7 @@ enum p9_req_status_t {
> struct p9_req_t {
> int status;
> int t_err;
> - wait_queue_head_t *wq;
> + wait_queue_head_t wq;
> struct p9_fcall *tc;
> struct p9_fcall *rc;
> void *aux;
> diff --git a/net/9p/client.c b/net/9p/client.c
> index b89c7298267c..bc8aba6b5ce0 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -282,8 +282,9 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
> return ERR_PTR(-ENOMEM);
> }
> for (col = 0; col < P9_ROW_MAXTAG; col++) {
> - c->reqs[row][col].status = REQ_STATUS_IDLE;
> - c->reqs[row][col].tc = NULL;
> + req = &c->reqs[row][col];
> + req->status = REQ_STATUS_IDLE;
> + init_waitqueue_head(&req->wq);
> }
> c->max_tag += P9_ROW_MAXTAG;
> }
> @@ -293,13 +294,6 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
> col = tag % P9_ROW_MAXTAG;
>
> req = &c->reqs[row][col];
> - if (!req->wq) {
> - req->wq = kmalloc(sizeof(wait_queue_head_t), GFP_NOFS);
> - if (!req->wq)
> - goto grow_failed;
> - init_waitqueue_head(req->wq);
> - }
> -
> if (!req->tc)
> req->tc = p9_fcall_alloc(alloc_msize);
> if (!req->rc)
> @@ -319,9 +313,7 @@ p9_tag_alloc(struct p9_client *c, u16 tag, unsigned int max_size)
> pr_err("Couldn't grow tag array\n");
> kfree(req->tc);
> kfree(req->rc);
> - kfree(req->wq);
> req->tc = req->rc = NULL;
> - req->wq = NULL;
> return ERR_PTR(-ENOMEM);
> }
>
> @@ -409,7 +401,6 @@ static void p9_tag_cleanup(struct p9_client *c)
> /* free requests associated with tags */
> for (row = 0; row < (c->max_tag/P9_ROW_MAXTAG); row++) {
> for (col = 0; col < P9_ROW_MAXTAG; col++) {
> - kfree(c->reqs[row][col].wq);
> kfree(c->reqs[row][col].tc);
> kfree(c->reqs[row][col].rc);
> }
> @@ -452,7 +443,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
> smp_wmb();
> req->status = status;
>
> - wake_up(req->wq);
> + wake_up(&req->wq);
> p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
> }
> EXPORT_SYMBOL(p9_client_cb);
> @@ -773,7 +764,7 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
> }
> again:
> /* Wait for the response */
> - err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
> + err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
>
> /*
> * Make sure our req is coherent with regard to updates in other
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 05006cbb3361..3e096c98313c 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -490,7 +490,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
> virtqueue_kick(chan->vq);
> spin_unlock_irqrestore(&chan->lock, flags);
> p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
> - err = wait_event_killable(*req->wq, req->status >= REQ_STATUS_RCVD);
> + err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
> /*
> * Non kernel buffers are pinned, unpin them
> */
