DPDK patches and discussions
 help / color / mirror / Atom feed
From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
To: "Van Haaren, Harry" <harry.van.haaren@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Phil Yang <Phil.Yang@arm.com>,
	Gavin Hu <Gavin.Hu@arm.com>, nd <nd@arm.com>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	nd <nd@arm.com>
Subject: Re: [dpdk-dev] Questions on service core feature implementation
Date: Tue, 10 Mar 2020 16:06:04 +0000	[thread overview]
Message-ID: <VE1PR08MB5149F7322C3AC4345BF9D41C98FF0@VE1PR08MB5149.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <MWHPR1101MB21578E75AD4841A666912344D7FF0@MWHPR1101MB2157.namprd11.prod.outlook.com>

<snip>

> > Hi Harry,
> 
> Hey Honnappa,
Thanks for your answers.

> 
> > 	I have few observations on service core feature implementation.
> >
> > I believe it is allowed to map/unmap a lcore to service while the
> > service is running (i.e. not just during initialization stage). Please
> > clarify otherwise.
> 
> Service-cores wasn't designed for this use case, but this "popped up"
> as the implementation was being finalized. Service cores' solves the problem
> of HW vs SW PMDs lcore requirements being different - and service cores is
> the abstraction to hide that lcore requirement difference.
> 
> So I don't think we made a very conscious decision on if this is allowed or not.
> 
> 
> > Assuming yes -
> >
> > In the 'service_run' function, the code to detect use of atomics seems
> > to result in allowing the mt-unsafe service to run on multiple cores.
> > Looking at the following code:
> >
> > 1   const int use_atomics = (service_mt_safe(s) == 0) &&
> > 2                (rte_atomic32_read(&s->num_mapped_cores) > 1);
> > 3   if (use_atomics) {
> > 4                if (!rte_atomic32_cmpset((uint32_t *)&s->execute_lock, 0,
> > 1))
> > 5                        return -EBUSY;
> > 6
> > 7                rte_service_runner_do_callback(s, cs, i);
> > 8                rte_atomic32_clear(&s->execute_lock);
> > 9   } else
> > 10             rte_service_runner_do_callback(s, cs, i);
> >
> > Let us say, on core1, after line 4, 'use_atomics' is set to 0. core1
> > is running the service function.
> > On the control plane let us say 'rte_service_map_lcore_set' is called
> > to map the service to an additional core, core2.
> > Now, on core2, after line 4, 'use_atomics' is set to 1. But since
> > core1 did not take execute_lock, core2 will take the lock and starts
> > running the service function. This should not be allowed since the
> > service is a mt- unsafe service.
> >
> > Please let me know if my assessment is correct.
> > A possible solution is to take the execute_lock all the time.
> 
> Yes agreed that your scenario seems to show an inconsistency.
> As per above, I'm not sure this use case was really the target of the service-
> cores design - but we as a community should identify if we want to support
> this use case.
> 
> We could either update docs explicitly dis-allowing the above situation - this
> puts the burden on applications to understand what services could be running,
> and if they're being scheduled already.
Agree with you, it will be difficult for the application to determine how the service is scheduled.

> 
> The other solution being to change the implementation to allow this use-case,
> which will likely have a performance impact for those already using service
> cores for its original function, by (as you suggested) taking execute_lock all the
> time (and paying the atomic cost for that..)
'num_mapped_cores' indicates the number of cores the service can run on. But, to be able to effectively avoid the use of atomic instructions, we need to know if a service is 'currently' running on another core. This information is not maintained by the library I think maintaining that will also introduce additional performance impacts. So, currently,
a) the atomics are avoided only when the user configures MT-Unsafe service to run on a single core.
b) if the user really wants to allow a MT-Unsafe service to run on multiple cores non-concurrently, atomics are used.

I suggest the following changes:
a) library will use atomics if the service is MT-Unsafe irrespective of the value of 'num_mapped_cores'
b) if the user wants to run a MT-Unsafe service on a single core and avoid using atomics, then he should set the service capability to RTE_SERVICE_CAP_MT_SAFE (This might result in some performance improvements since 'num_mapped_cores' will not be checked anymore).

Impact on the current users
a) if they have configured MT-Unsafe service to run on multiple cores, then they are already taking the hit of atomic instructions (and some additional cost of reading/checking the 'num_mapped_cores', they should see some perf improvements). No impact here.
b) if they have configured MT-Unsafe service to run on single core, then they might experience additional performance penalty. It might not be full penalty of using atomics as we will be removing some instructions. This needs to be measured. They can regain the performance loss by configuring their service RTE_SERVICE_CAP_MT_SAFE.

> 
> 
> > Thank you,
> > Honnappa
> 
> Regards, -HvH

      reply	other threads:[~2020-03-10 16:06 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-07  1:14 Honnappa Nagarahalli
2020-03-10 12:52 ` Van Haaren, Harry
2020-03-10 16:06   ` Honnappa Nagarahalli [this message]

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=VE1PR08MB5149F7322C3AC4345BF9D41C98FF0@VE1PR08MB5149.eurprd08.prod.outlook.com \
    --to=honnappa.nagarahalli@arm.com \
    --cc=Gavin.Hu@arm.com \
    --cc=Phil.Yang@arm.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=nd@arm.com \
    /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).