All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Carrillo, Erik G" <erik.g.carrillo@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"rsanford@akamai.com" <rsanford@akamai.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [PATCH v2 1/3] timer: add per-installer pending lists	for each lcore
Date: Tue, 29 Aug 2017 16:13:10 +0000	[thread overview]
Message-ID: <BE54F058557D9A4FAC1D84E2FC6D87570ED5DC1A@fmsmsx117.amr.corp.intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB9772584F23D8BE@IRSMSX103.ger.corp.intel.com>

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, August 29, 2017 5:57 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/3] timer: add per-installer pending lists
> for each lcore
> 
> Hi Gabriel,
> 
> >
> > Instead of each priv_timer struct containing a single skiplist, this
> > commit adds a skiplist for each enabled lcore to priv_timer. In the
> > case that multiple lcores repeatedly install timers on the same target
> > lcore, this change reduces lock contention for the target lcore's
> > skiplists and increases performance.
> 
> I am not an rte_timer expert, but there is one thing that worries me:
> It seems that complexity of timer_manage() has increased with that patch
> quite a bit:
> now  it has to check/process up to RTE_MAX_LCORE skiplists instead of one,
> also it has to somehow to properly sort up to RTE_MAX_LCORE lists of
> retrieved (ready to run) timers.
> Wouldn't all that affect it's running time?
> 

Yes, it would indeed increase it.

> I understand your intention to reduce lock contention, but I suppose at least
> it could be done in a configurable way.
> Let say allow user to specify  dimension of pending_lists[] at init phase or so.
> Then timer from lcore_id=N will endup in
> pending_lists[N%RTE_DIM(pendilng_list)].
> 

This is a neat idea, and seemingly would allow the original performance to be maintained for applications where contention is not an issue.   I'll look into this change, as it may address other developers' concerns as well - thanks for the suggestion.

> Another thought - might be better to divide pending timers list not by client
> (lcore) id, but by expiration time - some analog of  timer wheel or so.
> That, I think might greatly decrease the probability that timer_manage() and
> timer_add() will try to access the same list.
> From other side timer_manage() still would have to consume skip-lists one
> by one.
> Though I suppose that's quite radical change from what we have right now.
> Konstantin
> 

Thanks,
Gabriel

  reply	other threads:[~2017-08-29 16:14 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-23 14:47 [PATCH 0/3] *** timer library enhancements *** Gabriel Carrillo
2017-08-23 14:47 ` [PATCH 1/3] timer: add per-installer pending lists for each lcore Gabriel Carrillo
2017-08-25 20:27   ` [PATCH v2 " Gabriel Carrillo
2017-08-29 10:57     ` Ananyev, Konstantin
2017-08-29 16:13       ` Carrillo, Erik G [this message]
2017-08-29 15:11   ` [PATCH " Stephen Hemminger
2017-08-23 14:47 ` [PATCH 2/3] timer: handle timers installed from non-EAL threads Gabriel Carrillo
2017-08-25 20:27   ` [PATCH v2 " Gabriel Carrillo
2017-08-23 14:47 ` [PATCH 3/3] doc: update timer lib docs Gabriel Carrillo
2017-08-25 20:27   ` [PATCH v2 " Gabriel Carrillo
2017-08-23 15:02 ` [PATCH 0/3] *** timer library enhancements *** Wiles, Keith
2017-08-23 16:19   ` Carrillo, Erik G
2017-08-23 16:50     ` Wiles, Keith
2017-08-23 19:28       ` Carrillo, Erik G
2017-08-23 21:04         ` Wiles, Keith
2017-08-24 14:08           ` Carrillo, Erik G
2017-08-25 20:26 ` [PATCH v2 " Gabriel Carrillo
2017-09-13 22:05   ` [PATCH v3 0/3] timer library enhancement Erik Gabriel Carrillo
2017-09-13 22:05     ` [PATCH v3 1/3] timer: add multiple pending lists option for each lcore Erik Gabriel Carrillo
2017-09-13 22:05     ` [PATCH v3 2/3] timer: handle timers installed from non-EAL threads Erik Gabriel Carrillo
2017-09-13 22:05     ` [PATCH v3 3/3] doc: update timer lib docs Erik Gabriel Carrillo
2017-09-18 16:19       ` Mcnamara, John
2017-09-19 17:04         ` Carrillo, Erik G
2017-09-19 17:02     ` [PATCH v4 0/3] timer library enhancement Erik Gabriel Carrillo
2017-09-19 17:02       ` [PATCH v4 1/3] timer: add multiple pending lists option for each lcore Erik Gabriel Carrillo
2017-09-19 17:02       ` [PATCH v4 2/3] timer: handle timers installed from non-EAL threads Erik Gabriel Carrillo
2017-09-19 17:02       ` [PATCH v4 3/3] doc: update timer lib docs Erik Gabriel Carrillo
2017-10-11 20:42       ` [PATCH v4 0/3] timer library enhancement Thomas Monjalon
2018-04-04 22:42         ` Thomas Monjalon
2018-04-05 21:53           ` Carrillo, Erik G
2017-09-05 21:52 [PATCH v2 1/3] timer: add per-installer pending lists for each lcore Carrillo, Erik G

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=BE54F058557D9A4FAC1D84E2FC6D87570ED5DC1A@fmsmsx117.amr.corp.intel.com \
    --to=erik.g.carrillo@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=rsanford@akamai.com \
    --cc=stephen@networkplumber.org \
    --subject='Re: [PATCH v2 1/3] timer: add per-installer pending lists	for each lcore' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.