patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Ola Liljedahl <Ola.Liljedahl@arm.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>,
	Steve Capper <Steve.Capper@arm.com>, nd <nd@arm.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH v3 1/3] ring: read tail using atomic load
Date: Mon, 8 Oct 2018 11:36:31 +0530	[thread overview]
Message-ID: <20181008060629.GA5228@jerin> (raw)
In-Reply-To: <7A156041-23EC-4CCB-B129-3607AF34A992@arm.com>

-----Original Message-----
> Date: Sun, 7 Oct 2018 20:44:54 +0000
> From: Ola Liljedahl <Ola.Liljedahl@arm.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, Honnappa Nagarahalli
>  <Honnappa.Nagarahalli@arm.com>, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>, "Gavin Hu (Arm Technology China)"
>  <Gavin.Hu@arm.com>, Steve Capper <Steve.Capper@arm.com>, nd <nd@arm.com>,
>  "stable@dpdk.org" <stable@dpdk.org>
> Subject: Re: [PATCH v3 1/3] ring: read tail using atomic load
> user-agent: Microsoft-MacOutlook/10.11.0.180909
> 


Could you please fix the email client for inline reply.

https://www.kernel.org/doc/html/v4.19-rc7/process/email-clients.html


> 
> On 07/10/2018, 06:03, "Jerin Jacob" <jerin.jacob@caviumnetworks.com> wrote:
> 
>     In arm64 case, it will have ATOMIC_RELAXED followed by asm volatile ("":::"memory") of rte_pause().
>     I would n't have any issue, if the generated code code is same or better than the exiting case. but it not the case, Right?
> The existing case is actually not interesting (IMO) as it exposes undefined behaviour which allows the compiler to do anything. But you seem to be satisfied with "works for me, right here right now". I think the cost of avoiding undefined behaviour is acceptable (actually I don't think it even will be noticeable).

I am not convinced because of use of volatile in head and tail indexes.
For me that brings the defined behavior.That the reason why I shared
the generated assembly code. If you think other way, Pick any compiler
and see generated output.

And

Freebsd implementation of ring buffer(Which DPDK derived from), Don't have
such logic, See https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L108

See below too.

> 
> Skipping the compiler memory barrier in rte_pause() potentially allows for optimisations that provide much more benefit, e.g. hiding some cache miss latency for later loads. The DPDK ring buffer implementation is defined so to enable inlining of enqueue/dequeue functions into the caller, any code could immediately follow these calls.
> 
> From INTERNATIONAL STANDARD ©ISO/IEC ISO/IEC 9899:201x
> Programming languages — C
> 
> 5.1.2.4
> 4 Two expression evaluations conflict if one of them modifies a memory location and the other one reads or modifies the same memory location.
> 
> 25 The execution of a program contains a data race if it contains two conflicting actions in different threads, at least one of which is not atomic, and neither happens before the other. Any such data race results in undefined behavior.

IMO, Both condition will satisfy if the variable is volatile and 32bit read will atomic
for 32b and 64b machines. If not, the problem persist for generic case
as well(lib/librte_ring/rte_ring_generic.h)


I agree with you on C11 memory model semantics usage. The reason why I
propose name for the file as rte_ring_c11_mem.h as DPDK it self did not
had definitions for load acquire and store release semantics.
I was looking for taking load acquire and store release semantics
from C11 instead of creating new API like Linux kernel for FreeBSD(APIs
like  atomic_load_acq_32(), atomic_store_rel_32()). If the file name is your
concern then we could create new abstractions as well. That would help
exiting KNI problem as well.

I think, currently it mixed usage because, the same variable declaration
used for C11 vs non C11 usage.Ideally we wont need "volatile" for C11
case. Either we need to change only to C11 mode OR have APIs for 
atomic_load_acq_() and atomic_store_rel_() to allow both models like
Linux kernel and FreeBSD.

> 
> -- Ola
> 
> 
> 

  reply	other threads:[~2018-10-08  6:06 UTC|newest]

Thread overview: 125+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-06  1:18 [dpdk-stable] [PATCH] ring: fix c11 memory ordering issue Gavin Hu
2018-08-06  9:19 ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2018-08-08  1:39   ` Gavin Hu
2018-08-07  3:19 ` [dpdk-stable] [PATCH v2] " Gavin Hu
2018-08-07  5:56   ` He, Jia
2018-08-07  7:56     ` Gavin Hu
2018-08-08  3:07       ` Jerin Jacob
2018-08-08  7:23         ` Thomas Monjalon
     [not found]   ` <20180917074735.28161-1-gavin.hu@arm.com>
2018-09-17  7:47     ` [dpdk-stable] [PATCH v3 3/3] doc: add cross compile part for sample applications Gavin Hu
2018-09-17  9:48       ` Jerin Jacob
2018-09-17 10:28         ` Gavin Hu (Arm Technology China)
2018-09-17 10:34           ` Jerin Jacob
2018-09-17 10:55             ` Gavin Hu (Arm Technology China)
2018-09-17 10:49       ` [dpdk-stable] [PATCH v4] " Gavin Hu
2018-09-17 10:53         ` [dpdk-stable] [PATCH v5] " Gavin Hu
2018-09-18 11:00           ` Jerin Jacob
2018-09-19  0:33           ` [dpdk-stable] [PATCH v6] " Gavin Hu
2018-09-17  8:11     ` [dpdk-stable] [PATCH v4 1/4] bus/fslmc: fix undefined reference of memsegs Gavin Hu
2018-09-17  8:11       ` [dpdk-stable] [PATCH v4 2/4] ring: read tail using atomic load Gavin Hu
2018-09-20  6:41         ` Jerin Jacob
2018-09-25  9:26           ` Gavin Hu (Arm Technology China)
2018-09-17  8:11       ` [dpdk-stable] [PATCH v4 3/4] ring: synchronize the load and store of the tail Gavin Hu
2018-09-17  8:11       ` [dpdk-stable] [PATCH v4 4/4] ring: move the atomic load of head above the loop Gavin Hu
2018-10-27 14:21         ` Thomas Monjalon
2018-09-17  8:17   ` [dpdk-stable] [PATCH v3 1/3] ring: read tail using atomic load Gavin Hu
2018-09-17  8:17     ` [dpdk-stable] [PATCH v3 2/3] ring: synchronize the load and store of the tail Gavin Hu
2018-09-26  9:29       ` Gavin Hu (Arm Technology China)
2018-09-26  9:59         ` Justin He
2018-09-29 10:57       ` Jerin Jacob
2018-10-17  6:29       ` [dpdk-stable] [PATCH 1/2] " Gavin Hu
2018-10-17  6:29         ` [dpdk-stable] [PATCH 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-17  6:35         ` [dpdk-stable] [PATCH 1/2] ring: synchronize the load and store of the tail Gavin Hu (Arm Technology China)
2018-10-27 14:39           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
2018-10-27 15:00             ` Jerin Jacob
2018-10-27 15:13               ` Thomas Monjalon
2018-10-27 15:34                 ` Jerin Jacob
2018-10-27 15:48                   ` Thomas Monjalon
2018-10-29  2:51                   ` Gavin Hu (Arm Technology China)
2018-10-29  2:57                   ` Gavin Hu (Arm Technology China)
2018-10-29 10:16                     ` Jerin Jacob
2018-10-29 10:47                       ` Thomas Monjalon
2018-10-29 11:10                         ` Jerin Jacob
2018-11-03 20:12                 ` Mattias Rönnblom
2018-11-05 21:51                   ` Honnappa Nagarahalli
2018-11-06 11:03                     ` Mattias Rönnblom
     [not found]         ` <1540955698-209159-1-git-send-email-gavin.hu@arm.com>
2018-10-31  3:14           ` [dpdk-stable] [PATCH v2 " Gavin Hu
2018-10-31  3:14           ` [dpdk-stable] [PATCH v2 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-31  3:35         ` [dpdk-stable] [PATCH v2 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-10-31  3:35         ` [dpdk-stable] [PATCH v2 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-31  9:36           ` Thomas Monjalon
2018-10-31 10:27             ` Gavin Hu (Arm Technology China)
2018-09-17  8:17     ` [dpdk-stable] [PATCH v3 3/3] " Gavin Hu
2018-09-26  9:29       ` Gavin Hu (Arm Technology China)
2018-09-26 10:06         ` Justin He
2018-09-29  7:19           ` [dpdk-stable] [dpdk-dev] " Stephen Hemminger
2018-09-29 10:59       ` [dpdk-stable] " Jerin Jacob
2018-09-26  9:29     ` [dpdk-stable] [PATCH v3 1/3] ring: read tail using atomic load Gavin Hu (Arm Technology China)
2018-09-26 10:09       ` Justin He
2018-09-29 10:48     ` Jerin Jacob
2018-10-05  0:47       ` Gavin Hu (Arm Technology China)
2018-10-05  8:21         ` Ananyev, Konstantin
2018-10-05 11:15           ` Ola Liljedahl
2018-10-05 11:36             ` Ola Liljedahl
2018-10-05 13:44               ` Ananyev, Konstantin
2018-10-05 14:21                 ` Ola Liljedahl
2018-10-05 15:11                 ` Honnappa Nagarahalli
2018-10-05 17:07                   ` Jerin Jacob
2018-10-05 18:05                     ` Ola Liljedahl
2018-10-05 20:06                       ` Honnappa Nagarahalli
2018-10-05 20:17                         ` Ola Liljedahl
2018-10-05 20:29                           ` Honnappa Nagarahalli
2018-10-05 20:34                             ` Ola Liljedahl
2018-10-06  7:41                               ` Jerin Jacob
2018-10-06 19:44                                 ` Ola Liljedahl
2018-10-06 19:59                                   ` Ola Liljedahl
2018-10-07  4:02                                   ` Jerin Jacob
2018-10-07 20:11                                     ` Ola Liljedahl
2018-10-07 20:44                                     ` Ola Liljedahl
2018-10-08  6:06                                       ` Jerin Jacob [this message]
2018-10-08  9:22                                         ` Ola Liljedahl
2018-10-08 10:00                                           ` Jerin Jacob
2018-10-08 10:25                                             ` Ola Liljedahl
2018-10-08 10:33                                               ` Gavin Hu (Arm Technology China)
2018-10-08 10:39                                                 ` Ola Liljedahl
2018-10-08 10:41                                                   ` Gavin Hu (Arm Technology China)
2018-10-08 10:49                                                 ` Jerin Jacob
2018-10-10  6:28                                                   ` Gavin Hu (Arm Technology China)
2018-10-10 19:26                                                     ` Honnappa Nagarahalli
2018-10-08 10:46                                               ` Jerin Jacob
2018-10-08 11:21                                                 ` Ola Liljedahl
2018-10-08 11:50                                                   ` Jerin Jacob
2018-10-08 11:59                                                     ` Ola Liljedahl
2018-10-08 12:05                                                       ` Jerin Jacob
2018-10-08 12:20                                                         ` [dpdk-stable] [dpdk-dev] " Jerin Jacob
2018-10-08 12:30                                                           ` Ola Liljedahl
2018-10-09  8:53                                                             ` Olivier Matz
2018-10-09  3:16                                             ` [dpdk-stable] " Honnappa Nagarahalli
2018-10-08 14:43                                           ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2018-10-08 14:46                                             ` Ola Liljedahl
2018-10-08 15:45                                               ` Ola Liljedahl
2018-10-08  5:27                               ` [dpdk-stable] " Honnappa Nagarahalli
2018-10-08 10:01                                 ` Ola Liljedahl
2018-10-27 14:17     ` Thomas Monjalon
     [not found] <1541157688-40012-1-git-send-email-gavin.hu@arm.com>
     [not found] ` <1541066031-29125-1-git-send-email-gavin.hu@arm.com>
     [not found]   ` <1540981587-88590-1-git-send-email-gavin.hu@arm.com>
     [not found]     ` <1540956945-211373-1-git-send-email-gavin.hu@arm.com>
2018-10-31 10:26       ` [dpdk-stable] [PATCH v3 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-10-31 22:07         ` [dpdk-stable] [dpdk-dev] " Stephen Hemminger
2018-11-01  9:56           ` Gavin Hu (Arm Technology China)
2018-10-31 10:26       ` [dpdk-stable] [PATCH v3 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-11-01  9:53     ` [dpdk-stable] [PATCH v4 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-11-01  9:53     ` [dpdk-stable] [PATCH v4 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-11-01 17:26       ` Stephen Hemminger
2018-11-02  0:53         ` Gavin Hu (Arm Technology China)
2018-11-02  4:30           ` Honnappa Nagarahalli
2018-11-02  7:15             ` Gavin Hu (Arm Technology China)
2018-11-02  9:36               ` Thomas Monjalon
2018-11-02 11:23                 ` Gavin Hu (Arm Technology China)
2018-11-02 11:21   ` [dpdk-stable] [PATCH v5 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-11-05  9:30     ` Olivier Matz
2018-11-02 11:21   ` [dpdk-stable] [PATCH v5 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-11-02 11:43     ` Bruce Richardson
2018-11-03  1:19       ` Gavin Hu (Arm Technology China)
2018-11-03  9:34         ` Honnappa Nagarahalli
2018-11-05 13:17           ` Thomas Monjalon
2018-11-05 13:41             ` Jerin Jacob
2018-11-05  9:44         ` Olivier Matz
2018-11-05 13:36           ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181008060629.GA5228@jerin \
    --to=jerin.jacob@caviumnetworks.com \
    --cc=Gavin.Hu@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Ola.Liljedahl@arm.com \
    --cc=Steve.Capper@arm.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=nd@arm.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).