Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp967969imm; Wed, 1 Aug 2018 08:07:39 -0700 (PDT) X-Google-Smtp-Source: AAOMgpcG873jbuIboROEpUfOCc68W6cMID23VQLPhys7HAVWxzWu7dwhtdEBvwsHlf4bb+AYRSiS X-Received: by 2002:a62:c410:: with SMTP id y16-v6mr27263875pff.161.1533136059567; Wed, 01 Aug 2018 08:07:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1533136059; cv=none; d=google.com; s=arc-20160816; b=usZiX4bdZXdoIVcTACJ/UkgXHc9vwWGowGgl6Fgjna4Hv6J/L3tMyrQZvZ9wuc1mnB YCVJftbBpyoWjvY71Qc8xS4g2RW3x0YQh1UfL61M2pKWqzY+Vzc0f6qFfFjoSMVyNqWD x8GWIAyt537CTDZmOSqFrnn4aAMLXBKS2QCf6cTqsRDEOTKBTrWaWgRr3IHCYbvsjtqz SUfLYO7AJukxDeGNrIKxDqQcmsFPz9676gM55FmFtvExFiBpb81L6NgfxzrXEiKBssxe XtCu+OQ5IHGyDnFQGExEpNM3crScdmzWrqLilCVDJf+iW9GIjPYAcw2pQUprQCW5q0/8 noFA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date :arc-authentication-results; bh=3uOs8qqHpl/WC1SVx+q7u2vofFxWjommahk/z9UE96g=; b=jnOze6IOPjwXccTGVHw43znX68Rji1GexrnoZiE5YXmDxraBMr8pBse0VBBPExH9Be /IF6YXEsJw79hbJ88t8hAEj9Jhmnh6fU4NqR7TZrP/v77Qa+5G9TMsW/wnqDgvlCAmpo AnOoUKoTsQjRwxRdFXuuxkOuvT5Yel39GHCW2zIfmixwoF3XOv6WoKQ+yOp2ETR8Nn8m q62nNLqbuwAjM6p8hID+G/COv5GwIZ8Vp7Ub7JGVJ434b5zG9f/aE0OPOA+SZ2TvcnEb hKWahK3p9xE6cdN+l/yv7yJ8DrfDtkEgP0OIcs/HuaIrttlAlLCTD9TZR6jrKq4Wuu2Y jsNg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e186-v6si16594230pfc.176.2018.08.01.08.07.24; Wed, 01 Aug 2018 08:07:39 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389572AbeHAQwm (ORCPT + 99 others); Wed, 1 Aug 2018 12:52:42 -0400 Received: from 17.mo5.mail-out.ovh.net ([46.105.56.132]:51873 "EHLO 17.mo5.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389455AbeHAQwm (ORCPT ); Wed, 1 Aug 2018 12:52:42 -0400 Received: from player772.ha.ovh.net (unknown [10.109.146.19]) by mo5.mail-out.ovh.net (Postfix) with ESMTP id 181FF1D42BC for ; Wed, 1 Aug 2018 16:28:31 +0200 (CEST) Received: from bahia.lan (lns-bzn-46-82-253-208-248.adsl.proxad.net [82.253.208.248]) (Authenticated sender: groug@kaod.org) by player772.ha.ovh.net (Postfix) with ESMTPSA id 93F03740098; Wed, 1 Aug 2018 16:28:25 +0200 (CEST) Date: Wed, 1 Aug 2018 16:28:24 +0200 From: Greg Kurz To: Dominique Martinet Cc: v9fs-developer@lists.sourceforge.net, linux-fsdevel@vger.kernel.org, Matthew Wilcox , linux-kernel@vger.kernel.org Subject: Re: [V9fs-developer] [PATCH 2/2] net/9p: add a per-client fcall kmem_cache Message-ID: <20180801162824.31fb6a30@bahia.lan> In-Reply-To: <1532943263-24378-2-git-send-email-asmadeus@codewreck.org> References: <20180730093101.GA7894@nautica> <1532943263-24378-1-git-send-email-asmadeus@codewreck.org> <1532943263-24378-2-git-send-email-asmadeus@codewreck.org> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Ovh-Tracer-Id: 14091200286319024384 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: 0 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedtiedrledvgdejgecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecu Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 30 Jul 2018 11:34:23 +0200 Dominique Martinet wrote: > From: Dominique Martinet > > Having a specific cache for the fcall allocations helps speed up > allocations a bit, especially in case of non-"round" msizes. > > The caches will automatically be merged if there are multiple caches > of items with the same size so we do not need to try to share a cache > between different clients of the same size. > > Since the msize is negotiated with the server, only allocate the cache > after that negotiation has happened - previous allocations or > allocations of different sizes (e.g. zero-copy fcall) are made with > kmalloc directly. > > Signed-off-by: Dominique Martinet > --- The patch looks good to me. It would need to be rebased when you have fixed the potential kfree() of stale data in patch 1. Either with an extra goto label in p9_tag_alloc or by turning p9_fcall_alloc into p9_fcall_alloc_sdata, both solutions are equivalent. Just one suggestion, see below. > include/net/9p/client.h | 2 ++ > net/9p/client.c | 40 ++++++++++++++++++++++++++++++++-------- > net/9p/trans_rdma.c | 2 +- > 3 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/include/net/9p/client.h b/include/net/9p/client.h > index 4b4ac1362ad5..8d9bc7402a42 100644 > --- a/include/net/9p/client.h > +++ b/include/net/9p/client.h > @@ -123,6 +123,7 @@ struct p9_client { > struct p9_trans_module *trans_mod; > enum p9_trans_status status; > void *trans; > + struct kmem_cache *fcall_cache; > > union { > struct { > @@ -230,6 +231,7 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode, > kgid_t gid, struct p9_qid *); > int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status); > int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *fl); > +void p9_fcall_free(struct p9_client *c, struct p9_fcall *fc); > struct p9_req_t *p9_tag_lookup(struct p9_client *, u16); > void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status); > > diff --git a/net/9p/client.c b/net/9p/client.c > index ba99a94a12c9..215e3b1ed7b4 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -231,15 +231,34 @@ static int parse_opts(char *opts, struct p9_client *clnt) > return ret; > } > > -static int p9_fcall_alloc(struct p9_fcall *fc, int alloc_msize) > +static int p9_fcall_alloc(struct p9_client *c, struct p9_fcall *fc, > + int alloc_msize) > { > - fc->sdata = kmalloc(alloc_msize, GFP_NOFS); > + if (c->fcall_cache && alloc_msize == c->msize) This is a presumably hot path for any request but the initial TVERSION, you probably want likely() here... > + fc->sdata = kmem_cache_alloc(c->fcall_cache, GFP_NOFS); > + else > + fc->sdata = kmalloc(alloc_msize, GFP_NOFS); > if (!fc->sdata) > return -ENOMEM; > fc->capacity = alloc_msize; > return 0; > } > > +void p9_fcall_free(struct p9_client *c, struct p9_fcall *fc) > +{ > + /* sdata can be NULL for interrupted requests in trans_rdma, > + * and kmem_cache_free does not do NULL-check for us > + */ > + if (unlikely(!fc->sdata)) > + return; > + > + if (c->fcall_cache && fc->capacity == c->msize) ... and here as well. > + kmem_cache_free(c->fcall_cache, fc->sdata); > + else > + kfree(fc->sdata); > +} > +EXPORT_SYMBOL(p9_fcall_free); > + > static struct kmem_cache *p9_req_cache; > > /** > @@ -261,9 +280,9 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) > if (!req) > return NULL; > > - if (p9_fcall_alloc(&req->tc, alloc_msize)) > + if (p9_fcall_alloc(c, &req->tc, alloc_msize)) > goto free; > - if (p9_fcall_alloc(&req->rc, alloc_msize)) > + if (p9_fcall_alloc(c, &req->rc, alloc_msize)) > goto free; > > p9pdu_reset(&req->tc); > @@ -288,8 +307,8 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) > return req; > > free: > - kfree(req->tc.sdata); > - kfree(req->rc.sdata); > + p9_fcall_free(c, &req->tc); > + p9_fcall_free(c, &req->rc); > kmem_cache_free(p9_req_cache, req); > return ERR_PTR(-ENOMEM); > } > @@ -333,8 +352,8 @@ static void p9_free_req(struct p9_client *c, struct p9_req_t *r) > spin_lock_irqsave(&c->lock, flags); > idr_remove(&c->reqs, tag); > spin_unlock_irqrestore(&c->lock, flags); > - kfree(r->tc.sdata); > - kfree(r->rc.sdata); > + p9_fcall_free(c, &r->tc); > + p9_fcall_free(c, &r->rc); > kmem_cache_free(p9_req_cache, r); > } > > @@ -944,6 +963,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) > > clnt->trans_mod = NULL; > clnt->trans = NULL; > + clnt->fcall_cache = NULL; > > client_id = utsname()->nodename; > memcpy(clnt->name, client_id, strlen(client_id) + 1); > @@ -980,6 +1000,9 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) > if (err) > goto close_trans; > > + clnt->fcall_cache = kmem_cache_create("9p-fcall-cache", clnt->msize, > + 0, 0, NULL); > + > return clnt; > > close_trans: > @@ -1011,6 +1034,7 @@ void p9_client_destroy(struct p9_client *clnt) > > p9_tag_cleanup(clnt); > > + kmem_cache_destroy(clnt->fcall_cache); > kfree(clnt); > } > EXPORT_SYMBOL(p9_client_destroy); > diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c > index c5cac97df7f7..5e43f0a00b3a 100644 > --- a/net/9p/trans_rdma.c > +++ b/net/9p/trans_rdma.c > @@ -445,7 +445,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req) > if (unlikely(atomic_read(&rdma->excess_rc) > 0)) { > if ((atomic_sub_return(1, &rdma->excess_rc) >= 0)) { > /* Got one! */ > - kfree(req->rc.sdata); > + p9_fcall_free(client, &req->rc); > req->rc.sdata = NULL; > goto dont_need_post_recv; > } else {