DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Carrillo, Erik G" <erik.g.carrillo@intel.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	Phil Yang <Phil.Yang@arm.com>,
	"rsanford@akamai.com" <rsanford@akamai.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	Gavin Hu <Gavin.Hu@arm.com>, nd <nd@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update
Date: Fri, 24 Apr 2020 01:26:40 +0000	[thread overview]
Message-ID: <SA0PR11MB4656743DA70EF2884DA83AACB9D00@SA0PR11MB4656.namprd11.prod.outlook.com> (raw)
In-Reply-To: <DBBPR08MB4646967163244A5380431F7A98D30@DBBPR08MB4646.eurprd08.prod.outlook.com>

Hi Honnappa,

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Thursday, April 23, 2020 3:06 PM
> To: Phil Yang <Phil.Yang@arm.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> nd <nd@arm.com>
> Subject: RE: [PATCH v2] lib/timer: relax barrier for status update
> 
> Hi Erik,
> 
> > Subject: [PATCH v2] lib/timer: relax barrier for status update
> >
> > Volatile has no ordering semantics. The rte_timer structure defines
> > timer status as a volatile variable and uses the rte_r/wmb barrier to
> > guarantee inter-thread visibility.
> >
> > This patch optimized the volatile operation with c11 atomic operations
> > and one-way barrier to save the performance penalty. According to the
> > timer_perf_autotest benchmarking results, this patch can uplift
> > 10%~16% timer appending performance, 3%~20% timer resetting
> > performance and 45% timer callbacks scheduling performance on aarch64
> > and no loss in performance for x86.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> >
> > ---
> > This patch depends on patch:
> > http://patchwork.dpdk.org/patch/65997/
> >
> > v2:
> > 1. Changed the memory ordering comment in timer_set_config_state.
> > 2. It is still using built-ins as the wrapper functions for C11
> > built-ins are not defined yet.
> It is too late to get the wrapper functions done for 20.05. It was decided in
> yesterday's tech board meeting to go ahead with C11 atomic built-ins (since
> there is lot of code in DPDK that uses C11 built-ins). If there are no further
> comments, can you please provide your ack?
> 

Ok, thanks for letting me know.  Based on that decision,  I've taken another look 
and done some testing and it looks good to me.  I've made one comment in-line
below and acked it.

<... snipped ...>

> > @@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
> >  		 * mark it atomically as being configured */
> >  		status.state = RTE_TIMER_CONFIG;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* CONFIG states are acting as locked states. If the
> > +		 * timer is in CONFIG state, the state cannot be changed
> > +		 * by other threads. So, we should use ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +284,27 @@
> > timer_set_running_state(struct rte_timer *tim)
> >
> >  	/* wait that the timer is in correct status before update,
> >  	 * and mark it as running */
> > -	while (success == 0) {
> > -		prev_status.u32 = tim->status.u32;
> > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > __ATOMIC_RELAXED);
> >
> > +	while (success == 0) {
> >  		/* timer is not pending anymore */
> >  		if (prev_status.state != RTE_TIMER_PENDING)
> >  			return -1;
> >
> >  		/* here, we know that timer is stopped or pending,

We know that the timer will be pending at this point... Since we're correcting the comment below, we can correct this part too.

With that change:
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

> > -		 * mark it atomically as being configured */
> > +		 * mark it atomically as being running
> > +		 */
> >  		status.state = RTE_TIMER_RUNNING;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* RUNNING states are acting as locked states. If the
> > +		 * timer is in RUNNING state, the state cannot be changed
> > +		 * by other threads. So, we should use ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	return 0;

Thanks,
Erik

  reply	other threads:[~2020-04-24  1:27 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-24  6:42 [dpdk-dev] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock Phil Yang
2020-02-24  6:42 ` [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update Phil Yang
2020-04-08 10:23   ` Phil Yang
2020-04-08 21:10   ` Carrillo, Erik G
2020-04-08 21:16     ` Honnappa Nagarahalli
2020-04-08 21:26       ` Carrillo, Erik G
2020-04-08 21:56         ` Honnappa Nagarahalli
2020-04-09 19:29           ` Carrillo, Erik G
2020-04-10  4:39             ` Phil Yang
2020-04-20 16:05   ` [dpdk-dev] [PATCH v2] " Phil Yang
2020-04-23 20:06     ` Honnappa Nagarahalli
2020-04-24  1:26       ` Carrillo, Erik G [this message]
2020-04-24  7:27         ` Phil Yang
2020-04-24  7:24     ` [dpdk-dev] [PATCH v3] " Phil Yang
2020-04-25 17:17       ` Thomas Monjalon
2020-04-26  7:36         ` Phil Yang
2020-04-26 12:18           ` Carrillo, Erik G
2020-04-26 14:20             ` Phil Yang
2020-04-26 19:30               ` Carrillo, Erik G
2020-04-26 14:45       ` [dpdk-dev] [PATCH v4] " Phil Yang
2020-04-26 20:06         ` Thomas Monjalon
2020-04-25 14:36     ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2020-04-25 15:51       ` Phil Yang
2020-04-25 16:07         ` Thomas Monjalon
2020-02-25 22:26 ` [dpdk-dev] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock Carrillo, Erik G
2020-04-25 17:21   ` [dpdk-dev] [dpdk-stable] " 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=SA0PR11MB4656743DA70EF2884DA83AACB9D00@SA0PR11MB4656.namprd11.prod.outlook.com \
    --to=erik.g.carrillo@intel.com \
    --cc=Gavin.Hu@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Phil.Yang@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=nd@arm.com \
    --cc=rsanford@akamai.com \
    --cc=thomas@monjalon.net \
    /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).