From patchwork Tue Nov 11 13:22:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Taras Kondratiuk X-Patchwork-Id: 40572 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ee0-f71.google.com (mail-ee0-f71.google.com [74.125.83.71]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id C1603218DE for ; Tue, 11 Nov 2014 13:23:16 +0000 (UTC) Received: by mail-ee0-f71.google.com with SMTP id e51sf6826860eek.10 for ; Tue, 11 Nov 2014 05:23:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:references:in-reply-to:cc:subject:precedence :list-id:list-unsubscribe:list-archive:list-post:list-help :list-subscribe:errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-transfer-encoding; bh=QLfm/w/IPcD8Rcesl3I6UJeP2814rLb96EFSvsLkeCc=; b=iSHDp1+i/MMKPi4zVzJsCt9ZsmdX1Lycm++989ak7IzxaWcfNuloQ86vJ2/J93Er9k LaYnivP9U8o1JhmoBCxrYJuiP9SjjfB39mK2oEmL5TBxHHP4ufpnsayO7ht5MlUn/K5p Wp3oGmT6dbtSFPlQp1l03TF8V3bR7aNUXXp91bhybnABqOpYc5jBDnu8Qiid4cqry8Ic JBrE/MUt7vpgs1LP9mXRhJQ/ymGjdeOXqORSgoWmSaDjZ1HtioqveZgCB4BF4sL/TLvc 0MsGQXdsuNok9bpZQpgkz2V2CS8yMekPV4+qTtW5U6xtLZAyJcAMO4gZgwz1iJFavuX2 XzaA== X-Gm-Message-State: ALoCoQmG+1tEvG4DIolcP3XZ7ExY+lknwZjA62vAbn1eOuW9OeHE8O+YXKhwszHKwCH0P6OdnI+b X-Received: by 10.112.89.195 with SMTP id bq3mr311660lbb.9.1415712195822; Tue, 11 Nov 2014 05:23:15 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.121.72 with SMTP id li8ls619876lab.101.gmail; Tue, 11 Nov 2014 05:23:15 -0800 (PST) X-Received: by 10.112.199.100 with SMTP id jj4mr15691309lbc.71.1415712195155; Tue, 11 Nov 2014 05:23:15 -0800 (PST) Received: from mail-la0-f49.google.com (mail-la0-f49.google.com. [209.85.215.49]) by mx.google.com with ESMTPS id qs7si31408433lbb.76.2014.11.11.05.23.15 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 11 Nov 2014 05:23:15 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.49 as permitted sender) client-ip=209.85.215.49; Received: by mail-la0-f49.google.com with SMTP id ge10so9285962lab.8 for ; Tue, 11 Nov 2014 05:23:15 -0800 (PST) X-Received: by 10.152.6.228 with SMTP id e4mr36136453laa.71.1415712195067; Tue, 11 Nov 2014 05:23:15 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.184.201 with SMTP id ew9csp253165lbc; Tue, 11 Nov 2014 05:23:14 -0800 (PST) X-Received: by 10.236.63.163 with SMTP id a23mr36777198yhd.41.1415712193385; Tue, 11 Nov 2014 05:23:13 -0800 (PST) Received: from ip-10-35-177-41.ec2.internal (lists.linaro.org. [54.225.227.206]) by mx.google.com with ESMTPS id p12si36585470qaa.132.2014.11.11.05.23.12 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 11 Nov 2014 05:23:13 -0800 (PST) Received-SPF: none (google.com: lng-odp-bounces@lists.linaro.org does not designate permitted sender hosts) client-ip=54.225.227.206; Received: from localhost ([127.0.0.1] helo=ip-10-35-177-41.ec2.internal) by ip-10-35-177-41.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1XoBPb-0001WN-Al; Tue, 11 Nov 2014 13:23:11 +0000 Received: from mail-la0-f43.google.com ([209.85.215.43]) by ip-10-35-177-41.ec2.internal with esmtp (Exim 4.76) (envelope-from ) id 1XoBPU-0001Vq-IB for lng-odp@lists.linaro.org; Tue, 11 Nov 2014 13:23:04 +0000 Received: by mail-la0-f43.google.com with SMTP id ge10so9636384lab.16 for ; Tue, 11 Nov 2014 05:22:58 -0800 (PST) X-Received: by 10.152.88.46 with SMTP id bd14mr36496672lab.30.1415712178828; Tue, 11 Nov 2014 05:22:58 -0800 (PST) Received: from [172.22.39.11] ([195.238.92.128]) by mx.google.com with ESMTPSA id ny6sm5988148lbb.2.2014.11.11.05.22.58 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Nov 2014 05:22:58 -0800 (PST) Message-ID: <54620DB1.4010805@linaro.org> Date: Tue, 11 Nov 2014 15:22:57 +0200 From: Taras Kondratiuk User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Bill Fischofer References: <1415639447-27522-1-git-send-email-bill.fischofer@linaro.org> <5460F7F4.4020708@linaro.org> <5461C840.9050208@linaro.org> In-Reply-To: X-Topics: patch Cc: lng-odp-forward Subject: Re: [lng-odp] [PATCHv6 2/2] Initial review stage for ODP v1.0 buffer packet APIs X-BeenThere: lng-odp@lists.linaro.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: lng-odp-bounces@lists.linaro.org Sender: lng-odp-bounces@lists.linaro.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: taras.kondratiuk@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.49 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 On 11/11/2014 03:14 PM, Bill Fischofer wrote: > Good catch. Yes, the inner routines are missing an &. Will send out v2 > to correct that shortly. There are a few other issues: - ret_buf() operates with blk_freelist instead of buf_freelist - pool->s.bufcount should be set to 0 after buffers are pushed to a free list With fixes below buffer create/destroy works fine: > > Bill > > On Tue, Nov 11, 2014 at 2:26 AM, Taras Kondratiuk > > wrote: > > On 11/10/2014 08:07 PM, Bill Fischofer wrote: > > Thanks. See inline comments: > > > > On Mon, Nov 10, 2014 at 11:37 AM, Taras Kondratiuk > > > >> wrote: > > > > On 11/10/2014 07:10 PM, Bill Fischofer wrote: > > > > Signed-off-by: Bill Fischofer > > >> > > --- > > Versions of this patch > > == > > v1 - Original (missing files) > > v2 - Added missing odp_buffer_inlines.h > > v3 - Added missing odph_tcp.h > > v4 - (mispublished as v3): Allow NULL arguments to > > odp_buffer_pool_create() > > v5 - Correctly handle timer allocs (no blocks), add > support for > > buf_init calls > > on allocation. > > v6 - Add remaining segment APIs + unmaps + additional > (void *) > > casts needed > > for some C compiler environments, as noted by Taras > > > > > > > > + > > + pool->s.bufcount = 0; > > + pool->s.buf_freelist = NULL; > > + pool->s.blk_freelist = NULL; > > > > > > Looks like pool->s.blk_freelist and pool->s.buf_freelist are not > > updated anywhere. And then dereferenced in ret_buf(). > > > > > > They are initialized in the lines just above your comment. Not sure > > what problem you see here. The ret_buf()/ret_blk() routines push > > buffers/blocks onto their respective freelists, so the contents > of these > > pointers become the next pointer for the freed elements. Result > is that > > the NULL propagates to the last element of the list, and is > restored to > > the list anchor when the last element on that list is popped off > of it. > > These are push/pop stack structures. > > That's is not how it works right now. Current code does: > 1. pool->s.blk_freelist = NULL; > 2. later odp_cs(pool->blk_freelist, oldhead, newhead) -> > __sync_bool_compare_and_swap(pool->blk_freelist, oldhead, > newhead) > 3. __sync_bool_compare_and_swap() dereferences blk_freelist and get > segfault, because it is NULL. > > To get behavior that you have described a *pointer* to blk_freelist > have to passed in odp_cs instead of blk_freelist itself. > > -odp_cs(pool->blk_freelist, oldhead, newhead) > +odp_cs(&pool->blk_freelist, oldhead, newhead) > > > > > > > + > > + uint8_t *buf = pool->s.udata_base_addr - buf_stride; > > > > > > pool->s.udata_base_addr is not initialized here. > > Should it be just udata_base_addr? > > > > > > No, it is initialized a few lines above in the statement: > > uint8_t *udata_base_addr = pool->s.pool_base_addr + mdata_size; > > uint8_t *block_base_addr = udata_base_addr + udata_size; > > That's the point. Local udata_base_addr is initialized, but then > uninitialized pool->s.udata_base_addr is used, which leads to segfault. > > Maybe you have sent not the latest patch? > > diff --git a/platform/linux-generic/include/odp_buffer_pool_internal.h b/platform/linux-generic/include/odp_buffer_pool_internal.h index 28ef8f2..07d920a 100644 --- a/platform/linux-generic/include/odp_buffer_pool_internal.h +++ b/platform/linux-generic/include/odp_buffer_pool_internal.h @@ -64,7 +64,6 @@ struct pool_entry_s { uint8_t *pool_base_addr; size_t pool_size; int mdata_stride; - uint8_t *udata_base_addr; int buf_udata_size; int udata_stride; odp_buffer_hdr_t *buf_freelist; @@ -89,11 +88,11 @@ extern void *pool_entry_ptr[]; #if UINTPTR_MAX == 0xffffffffffffffff #define odp_at odp_atomic_u64_t -#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(p), \ +#define odp_cs(p, o, n) odp_atomic_cmpset_u64((odp_at *)(&p), \ (uint64_t)(o), (uint64_t)(n)) #else #define odp_at odp_atomic_u32_t -#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(p), \ +#define odp_cs(p, o, n) odp_atomic_cmpset_u32((odp_at *)(&p), \ (uint32_t)(o), (uint32_t)(n)) #endif @@ -154,7 +153,7 @@ static inline void ret_buf(struct pool_entry_s *pool, odp_buffer_hdr_t *buf) do { oldhead = odp_ref(pool->buf_freelist); buf->next = oldhead; - } while (odp_cs(pool->blk_freelist, oldhead, buf) == 0); + } while (odp_cs(pool->buf_freelist, oldhead, buf) == 0); odp_atomic_dec_u32(&pool->bufcount); } diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index 39aed56..a45625f 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -221,11 +221,10 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, uint8_t *udata_base_addr = pool->s.pool_base_addr + mdata_size; uint8_t *block_base_addr = udata_base_addr + udata_size; - pool->s.bufcount = 0; pool->s.buf_freelist = NULL; pool->s.blk_freelist = NULL; - uint8_t *buf = pool->s.udata_base_addr - buf_stride; + uint8_t *buf = udata_base_addr - buf_stride; uint8_t *udat = (udata_stride == 0) ? NULL : block_base_addr - udata_stride; @@ -247,6 +246,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, udat -= udata_stride; } while (buf >= pool->s.pool_base_addr); + pool->s.bufcount = 0; + /* Form block freelist for pool */ uint8_t *blk = pool->s.pool_base_addr + pool->s.pool_size - pool->s.seg_size;