From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 23E37A051D; Fri, 26 Jun 2020 16:13:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C00AC1BFC8; Fri, 26 Jun 2020 16:13:31 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id 71CA71BFC3 for ; Fri, 26 Jun 2020 16:13:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593180809; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=QZ6xQzMANdKLwTtORZv4KYGiDt3xMpC7xx8dFyGrpjU=; b=Hf6vaPmgVAQX6LBXjl5nFCHFR45nIqbQSMPg48qJi9JVAfdqPYLpgk2IK1SV43hRqosjAo iAuHWX6lw9RNBkyzIhzIa1dA5N7Ov7xjZtRV4ZPIWWckcO/QKufFJgb1rbTSbzy+uqKTBz p5E3mD+kcBmbO4vThLqi/JGlD9IlawI= Received: from mail-vs1-f71.google.com (mail-vs1-f71.google.com [209.85.217.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-419-gkvEJYcpNBqzgExCO423nw-1; Fri, 26 Jun 2020 10:13:24 -0400 X-MC-Unique: gkvEJYcpNBqzgExCO423nw-1 Received: by mail-vs1-f71.google.com with SMTP id h27so3135518vsj.17 for ; Fri, 26 Jun 2020 07:13:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=QZ6xQzMANdKLwTtORZv4KYGiDt3xMpC7xx8dFyGrpjU=; b=oaRz4meBbv4Z8XkFJ330bNF//zGj1WuUupIPMYJBRIDepCS7nNhtKWvtJifbPBTyRI SjMMmpk0pPVNRLpCYtDtwm7FrXup/zJCe8uFm5FFJpAcDmzb6fvjsmZzSa0z5pCxI1Wd LeHL9xZVsXshCiogVAsiuW4kXVxr1AwiO4Lyet21pLo2y+9Dr95mj0P8DUCK7OJeYakZ 4pwrrAkz9mkkCAJABIR0tOWA6w1KMF7NcMnkNG6Mh0l0/6xPIxc2YsXk2Sps75CSeHRE iAjKpdJDNjrXBI0+yAldT5yjoPnCs5i+fLxTheZH0d+IhMg2OeH/ZYmI89OWKVBZJhKc J07w== X-Gm-Message-State: AOAM5333201ikLr+3yCNwvxFJeWp06NxuGkbA+V7ZeU41eXy5t1LwLkX DFQmu3R4gcHYIMooDGZW7qpdAspDbF4RJgTh3yoc0LvSO6r5wZk66yKIf1XH4j0eOb5Px8USj3z MT5UZX/M/MQHuIAnwE/w= X-Received: by 2002:a67:2fc6:: with SMTP id v189mr2661330vsv.198.1593180804118; Fri, 26 Jun 2020 07:13:24 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyEz7B5PlayZlJiciwj6tRsWylAQjoMsP+iIs5wXtQKyW8QHdO5rPzo1TwdtP+QGolHIZwmYYVl9J6eNJs/JqQ= X-Received: by 2002:a67:2fc6:: with SMTP id v189mr2661291vsv.198.1593180803715; Fri, 26 Jun 2020 07:13:23 -0700 (PDT) MIME-Version: 1.0 References: <20200610144506.30505-1-david.marchand@redhat.com> <20200622132531.21857-1-david.marchand@redhat.com> <20200622132531.21857-10-david.marchand@redhat.com> <41b560ac-5087-55a3-3e05-ee7903e7fabe@solarflare.com> In-Reply-To: <41b560ac-5087-55a3-3e05-ee7903e7fabe@solarflare.com> From: David Marchand Date: Fri, 26 Jun 2020 16:13:12 +0200 Message-ID: To: Andrew Rybchenko Cc: dev , Jerin Jacob , Bruce Richardson , Ray Kinsella , Kevin Traynor , Ian Stokes , Ilya Maximets , "Artem V. Andreev" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 9/9] mempool/bucket: handle non-EAL lcores X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jun 23, 2020 at 7:28 PM Andrew Rybchenko wrote: > > On 6/22/20 4:25 PM, David Marchand wrote: > > Convert to new lcore API to support non-EAL lcores. > > > > Signed-off-by: David Marchand > > --- > > drivers/mempool/bucket/rte_mempool_bucket.c | 131 ++++++++++++-------- > > 1 file changed, 82 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/mempool/bucket/rte_mempool_bucket.c b/drivers/mempool/bucket/rte_mempool_bucket.c > > index 5ce1ef16fb..0b4f42d330 100644 > > --- a/drivers/mempool/bucket/rte_mempool_bucket.c > > +++ b/drivers/mempool/bucket/rte_mempool_bucket.c > > @@ -55,6 +55,7 @@ struct bucket_data { > > struct rte_ring *shared_orphan_ring; > > struct rte_mempool *pool; > > unsigned int bucket_mem_size; > > + void *lcore_callback_handle; > > }; > > > > static struct bucket_stack * > > @@ -345,6 +346,22 @@ bucket_dequeue_contig_blocks(struct rte_mempool *mp, void **first_obj_table, > > return 0; > > } > > > > +struct bucket_per_lcore_ctx { > > The structure is not used in per-lcore init and uninit > functions. So, it is better to add _count to make it > count specified. I.e. bucket_count_per_lcore_ctx. Yes, and this aligns with its only user being renamed from count_per_lcore to bucket_count_per_lcore. > > > + const struct bucket_data *bd; > > + unsigned int count; > > +}; > > + > > +static int > > +count_per_lcore(unsigned int lcore_id, void *arg) > > +{ > > + struct bucket_per_lcore_ctx *ctx = arg; > > + > > + ctx->count += ctx->bd->obj_per_bucket * > > + ctx->bd->buckets[lcore_id]->top; > > + ctx->count += rte_ring_count(ctx->bd->adoption_buffer_rings[lcore_id]); > > + return 0; > > +} > > + > > static void > > count_underfilled_buckets(struct rte_mempool *mp, > > void *opaque, > > @@ -373,23 +390,66 @@ count_underfilled_buckets(struct rte_mempool *mp, > > static unsigned int > > bucket_get_count(const struct rte_mempool *mp) > > { > > - const struct bucket_data *bd = mp->pool_data; > > - unsigned int count = > > - bd->obj_per_bucket * rte_ring_count(bd->shared_bucket_ring) + > > - rte_ring_count(bd->shared_orphan_ring); > > - unsigned int i; > > + struct bucket_per_lcore_ctx ctx; > > Just a nit, but I think that ctx is too generic. > (some time ago bucket_data bd was ctx in fact :) ) > May be bplc? Up to you. Ack. > > > > > - for (i = 0; i < RTE_MAX_LCORE; i++) { > > - if (!rte_lcore_is_enabled(i)) > > - continue; > > - count += bd->obj_per_bucket * bd->buckets[i]->top + > > - rte_ring_count(bd->adoption_buffer_rings[i]); > > - } > > + ctx.bd = mp->pool_data; > > + ctx.count = ctx.bd->obj_per_bucket * > > + rte_ring_count(ctx.bd->shared_bucket_ring); > > + ctx.count += rte_ring_count(ctx.bd->shared_orphan_ring); > > > > + rte_lcore_iterate(count_per_lcore, &ctx); > > rte_mempool_mem_iter((struct rte_mempool *)(uintptr_t)mp, > > - count_underfilled_buckets, &count); > > + count_underfilled_buckets, &ctx.count); > > + > > + return ctx.count; > > +} > > + > > +static int > > +bucket_init_per_lcore(unsigned int lcore_id, void *arg) > > It should be no bucket_ prefix here, or it should be bucket_ > prefix above in count_per_lcore. As mentioned before, ack. > > > +{ > > + char rg_name[RTE_RING_NAMESIZE]; > > + struct bucket_data *bd = arg; > > + struct rte_mempool *mp; > > + int rg_flags; > > + int rc; > > + > > + mp = bd->pool; > > + bd->buckets[lcore_id] = bucket_stack_create(mp, > > + mp->size / bd->obj_per_bucket); > > + if (bd->buckets[lcore_id] == NULL) > > + goto error; > > + > > + rc = snprintf(rg_name, sizeof(rg_name), RTE_MEMPOOL_MZ_FORMAT ".a%u", > > + mp->name, lcore_id); > > + if (rc < 0 || rc >= (int)sizeof(rg_name)) > > + goto error; > > + > > + rg_flags = RING_F_SC_DEQ; > > + if (mp->flags & MEMPOOL_F_SP_PUT) > > + rg_flags |= RING_F_SP_ENQ; > > + if (mp->flags & MEMPOOL_F_SC_GET) > > + rg_flags |= RING_F_SC_DEQ; > > There is not point to have two above lines here, since > RING_F_SC_DEQ is always set. Ah yes, I did not realise when moving the code. > > > + bd->adoption_buffer_rings[lcore_id] = rte_ring_create(rg_name, > > + rte_align32pow2(mp->size + 1), mp->socket_id, rg_flags); > > + if (bd->adoption_buffer_rings[lcore_id] == NULL) > > + goto error; > > > > - return count; > > + return 0; > > +error: > > + rte_free(bd->buckets[lcore_id]); > > + bd->buckets[lcore_id] = NULL; > > + return -1; > > Why does the API collapse all negative errnos into -1? > (I don't think it is critical, just want to know why). I collapsed everything as a single error as we have a partial idea of what went wrong when calling this callback with all lcores at registration. We could get a specific error reported by this callback, but then we would not know on which lcore (programmatically). And in the end, all errors will summarize as a lack of resources, I do not expect a need for input validation. Maybe you have other use cases in mind? Thanks for the review. -- David Marchand