From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f175.google.com (mail-pf0-f175.google.com [209.85.192.175]) by dpdk.org (Postfix) with ESMTP id AB8F4595E for ; Thu, 5 May 2016 23:28:24 +0200 (CEST) Received: by mail-pf0-f175.google.com with SMTP id c189so43124248pfb.3 for ; Thu, 05 May 2016 14:28:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=6GAR4BoGpxGg4OKVQPsU3WFiXEh1fh60pL8w6pOhfXw=; b=kb1dLn+Hxiv5Qx4aT2uIIgpS1bIfVhjCyaLI3/h6DNsI3uQjPzMZMl6A0llitwi7C4 N5s116xVgMEqLMfyoGWyfMStvJV97tIGPFRBPmqI4ZpFWs0jrDXz9uNhvXtWP3+ekn9M mRJTzzfpV8vRUHAjB3Oz6gueig5ZU0eY8VV3E8M+xEXVcgh/SeLIabUBibmR+wTDlHxE n+xkdakzknXX2FDCt4wBbjgr91QT2nfekmPoWnrxVdXMy8kjZ/gT9QWMK3lbir9dKMwc tYTqytxitkhLwpbnDnjVEPG1ZR93ByCQspd5OmLFRPzxxpxnzepOmhTN4U1sNWyg4h9w 9ZjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=6GAR4BoGpxGg4OKVQPsU3WFiXEh1fh60pL8w6pOhfXw=; b=fBrI10LnzrrCmCsX+Q+anJH755ebgu5Is4f9RAVI2XIeGMkOhAa5RdoJ89RUfnEt8O Vtr0LzKJaWfH2bruGzdlaY/GCzME2Vb/Ru84CAT/cER6s/le3Uo+OwmHzc97ZX9w/X4+ Dc0nbYEmbkQ5mABeKKBKKBsRbwxHOHDp8z2JRdg2l/M33cqmosa1pU8AR94euUqY0DwC sna8qreacB+NQeKgCoE/7bXM8kiNJxoywj7H6JcknRwIxDjkWCr019W7vybCvwghHIZ8 WHA8tHV7+KJumwuvp5gv+SvPCnZUHMSxsku6u+kMOqel9R9kz9BlB3DMCWcwqP5y4bLU +w/w== X-Gm-Message-State: AOPr4FW6k3dgN+OM+zrexUhlRwUN1NjFpWC4lrnSGHoRS+E3V4xfxMGSylEM5IA0WROv6Q== X-Received: by 10.98.7.24 with SMTP id b24mr23515027pfd.125.1462483703966; Thu, 05 May 2016 14:28:23 -0700 (PDT) Received: from xeon-e3 (static-50-53-72-186.bvtn.or.frontiernet.net. [50.53.72.186]) by smtp.gmail.com with ESMTPSA id ut1sm15851257pac.46.2016.05.05.14.28.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 May 2016 14:28:23 -0700 (PDT) Date: Thu, 5 May 2016 14:28:35 -0700 From: Stephen Hemminger To: David Hunt Cc: olivier.matz@6wind.com, dev@dpdk.org Message-ID: <20160505142835.7df35079@xeon-e3> In-Reply-To: <1462472982-49782-2-git-send-email-david.hunt@intel.com> References: <1462472982-49782-1-git-send-email-david.hunt@intel.com> <1462472982-49782-2-git-send-email-david.hunt@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/2] mempool: add stack (fifo) mempool handler X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 May 2016 21:28:25 -0000 Overall, this is ok, but why can't it be the default? Lots of little things should be cleaned up > +struct rte_mempool_common_stack > +{ > + /* Spinlock to protect access */ > + rte_spinlock_t sl; > + > + uint32_t size; > + uint32_t len; > + void *objs[]; > + > +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG > +#endif Useless remove it > +}; > + > +static void * > +common_stack_alloc(struct rte_mempool *mp) > +{ > + struct rte_mempool_common_stack *s; > + char stack_name[RTE_RING_NAMESIZE]; > + unsigned n = mp->size; > + int size = sizeof(*s) + (n+16)*sizeof(void*); > + > + /* Allocate our local memory structure */ > + snprintf(stack_name, sizeof(stack_name), "%s-common-stack", mp->name); > + s = rte_zmalloc_socket(stack_name, The name for zmalloc is ignored in current code, why bother making it unique. > + size, > + RTE_CACHE_LINE_SIZE, > + mp->socket_id); > + if (s == NULL) { > + RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n"); > + return NULL; > + } > + > + /* And the spinlock we use to protect access */ > + rte_spinlock_init(&s->sl); > + > + s->size = n; > + mp->pool = (void *) s; Since pool is void *, no need for a cast here > + rte_mempool_set_handler(mp, "stack"); > + > + return (void *) s; > +} > + > +static int common_stack_put(void *p, void * const *obj_table, > + unsigned n) > +{ > + struct rte_mempool_common_stack * s = (struct rte_mempool_common_stack *)p; > + void **cache_objs; > + unsigned index; > + > + /* Acquire lock */ Useless obvious comment, about as good a classic /* Add one to i */ > > +static int common_stack_get(void *p, void **obj_table, > + unsigned n) > +{ > + struct rte_mempool_common_stack * s = (struct rte_mempool_common_stack *)p; Unnecessary cast, in C void * can be assigned to any type. > + void **cache_objs; > + unsigned index, len; > + > + /* Acquire lock */ Yet another useless comment. > + rte_spinlock_lock(&s->sl); > + > + if(unlikely(n > s->len)) { > + rte_spinlock_unlock(&s->sl); > + return -ENOENT; > + } > + > + cache_objs = s->objs; > + > + for (index = 0, len = s->len - 1; index < n; ++index, len--, obj_table++) > + *obj_table = cache_objs[len]; > + > + s->len -= n; > + rte_spinlock_unlock(&s->sl); > + return n; > +} > + > +static unsigned common_stack_get_count(void *p) > +{ > + struct rte_mempool_common_stack * s = (struct rte_mempool_common_stack *)p; Another useless cast. > + return s->len; > +} > + > +static void > +common_stack_free(void *p) > +{ > + rte_free((struct rte_mempool_common_stack *)p); Yet another useless cast > +} > + > +static struct rte_mempool_handler handler_stack = { For security, any data structure with function pointers should be const. > + .name = "stack", > + .alloc = common_stack_alloc, > + .free = common_stack_free, > + .put = common_stack_put, > + .get = common_stack_get, > + .get_count = common_stack_get_count > +}; > + > +MEMPOOL_REGISTER_HANDLER(handler_stack);