From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>,
"Tan, Jianfeng" <jianfeng.tan@intel.com>
Cc: "Van Haaren, Harry" <harry.van.haaren@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request API to DPDK IPC
Date: Wed, 28 Mar 2018 11:42:23 +0100 [thread overview]
Message-ID: <5a7e8c12-9336-678a-ccdb-4938bb64e9c2@intel.com> (raw)
In-Reply-To: <3288381.S1gyJAqTvN@xps>
On 28-Mar-18 10:53 AM, Thomas Monjalon wrote:
> 28/03/2018 11:21, Burakov, Anatoly:
>> On 28-Mar-18 9:55 AM, Tan, Jianfeng wrote:
>>> On 3/28/2018 4:22 PM, Van Haaren, Harry wrote:
>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>> 28/03/2018 04:08, Tan, Jianfeng:
>>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>>>> 27/03/2018 15:59, Anatoly Burakov:
>>>>>>>> Under the hood, we create a separate thread to deal with replies to
>>>>>>>> asynchronous requests, that will just wait to be notified by the
>>>>>>>> main thread, or woken up on a timer.
>>>>>>>
>>>>>>> I really don't like that a library is creating a thread.
>>>>>>> We don't even know where the thread is created (which core).
>>>>>>> Can it be a rte_service? or in the interrupt thread?
>>>>>>
>>>>>> Agree that we'd better not adding so many threads in a library.
>>>>>>
>>>>>> I was considering to merge all the threads into the interrupt thread,
>>>>> however, we don't have an interrupt thread in freebsd. Further, we don't
>>>>> implement alarm API in freebsd. That's why I tend to current
>>>>> implementation,
>>>>> and optimize it later.
>>>>>
>>>>> I would prefer we improve the current code now instead of polluting more
>>>>> with more uncontrolled threads.
>>>>>
>>>>>> For rte_service, it may be not a good idea to reply on it as it needs
>>>>> explicit API calls to setup.
>>>>>
>>>>> I don't see the issue of the explicit API.
>>>>> The IPC is a new service.
>>>
>>> My concern is that not every DPDK application sets up rte_service, but
>>> IPC will be used for very fundamental functions, like memory allocation.
>>> We could not possibly ask all DPDK applications to add rte_service now.
>>>
>>> And also take Harry's comments below into consideration, most likely, we
>>> will move these threads into interrupt thread now by adding
>>>
>>>> Although I do like to see new services, if we want to enable "core"
>>>> dpdk functionality with Services, we need a proper designed solution
>>>> for that. Service cores is not intended for "occasional" work - there
>>>> is no method to block and sleep on a specific service until work
>>>> becomes available, so this would imply a busy-polling. Using a service
>>>> (hence busy polling) for rte_malloc()-based memory mapping requests is
>>>> inefficient, and total overkill :)
>>>>
>>>> For this patch I suggest to use some blocking-read capable mechanism.
>>>
>>> The problem here is that we add too many threads; blocking-read does not
>>> decrease # of threads.
>>>
>>>>
>>>> The above said, in the longer term it would be good to have a design
>>>> that allows new file-descriptors to be added to a "dpdk core" thread,
>>>> which performs occasional lengthy work if the FD has data available.
>>>
>>> Interrupt thread vs rte_service, which is the direction to go? We
>>> actually have some others threads, in vhost and even virtio-user; we can
>>> also avoid those threads if we have a clear direction.
>>>
>>> Thanks,
>>> Jianfeng
>>>
>>
>> Hi all,
>>
>> First of all, @Thomas, this is not a "new library" - it's part of EAL.
>
> I did not say it is a new library.
>
>> We're going to be removing a few threads from EAL as it is because of
>> IPC (Jianfeng has already submitted patches for those),
>
> I don't understand.
> Which threads are you going to remove? Which patch?
There are separate patches that get rid of some EAL threads we have from
before (e.g. VFIO thread) - this is the reason IPC was created in the
first place. These aren't relevant to this patchset per se, i'm just
saying that it's not like we're adding to the pile of already existing
threads without taking anything away :)
>
>> so i don't think
>> it's such a big deal to have two IPC threads instead of one. I'm open to
>> suggestions on how to make this work without a second thread, but i
>> don't see it.
>
> I am not against the second thread.
> I am against both threads :)
Well, the first thread is already in DPDK. To provide some context,
first implementation for DPDK IPC was suggested for 17.11, and (without
many conceptual changes) was merged in 18.02. I think it's a bit late to
be against both threads :)
>
>> We've discussed possibility of using rte_service internally, but decided
>> against it for reasons already outlined by Harry - it's not a suitable
>> mechanism for this kind of thing, not as it is.
>>
>> Using interrupt thread for this _will_ work, however this will require a
>> a lot more changes, as currently alarm API allocates everything through
>> rte_malloc, while we want to use IPC for rte_malloc (which would make it
>> a circular dependency). So it'll probably be more API and more
>> complexity for dealing with malloc vs rte_malloc allocations. Hence the
>> least-bad approach taken here: a new thread.
>
> If everybody is happy enough with "least bad" design and not trying
> to improve the core design, what can I say?
I'm not against trying to improve the core design. I'm just saying that,
had this kind of feedback been provided just a bit earlier, I would've
had time to fix it in time for deadlines. However, because memory rework
patchset depends on this API, i would suggest merging it in now, as is,
and commit to a roadmap of improvements for next release(s).
For starters, we could plan on removing alarm thread's dependency on
rte_malloc and just use regular malloc API's in there, and rework
asynchronous IPC API to use that instead. This shouldn't be much work,
and will presumably make you halfway happy, as one of the threads will
be gone :)
We can then look into removing the second thread and moving the entirety
of DPDK IPC into the interrupt thread. I'm not too sure how would that
work, but i haven't looked at it in any detail, so maybe it is feasible.
Can we agree on this? It would be great to do everything perfectly from
the first try, but having a goal in sight and working towards it is fine
too, even if not all of the steps we take are perfect.
--
Thanks,
Anatoly
next prev parent reply other threads:[~2018-03-28 10:42 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-27 14:59 [dpdk-dev] [PATCH] " Anatoly Burakov
2018-02-28 10:22 ` Burakov, Anatoly
2018-03-02 18:06 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2018-03-07 16:57 ` [dpdk-dev] [PATCH v3] " Anatoly Burakov
2018-03-13 17:42 ` [dpdk-dev] [PATCH v4] " Anatoly Burakov
2018-03-23 15:38 ` Tan, Jianfeng
2018-03-23 18:21 ` Burakov, Anatoly
2018-03-24 13:22 ` Burakov, Anatoly
2018-03-24 12:46 ` [dpdk-dev] [PATCH v5 1/2] eal: rename IPC sync request to pending request Anatoly Burakov
2018-03-26 7:31 ` Tan, Jianfeng
2018-03-27 13:59 ` [dpdk-dev] [PATCH v6 " Anatoly Burakov
2018-03-27 16:27 ` Thomas Monjalon
2018-03-28 9:15 ` Burakov, Anatoly
2018-03-28 10:08 ` Thomas Monjalon
2018-03-28 10:57 ` Burakov, Anatoly
2018-03-31 17:06 ` [dpdk-dev] [PATCH v7 1/3] " Anatoly Burakov
2018-03-31 17:06 ` [dpdk-dev] [PATCH v7 2/3] eal: rename mp_request to mp_request_sync Anatoly Burakov
2018-04-02 5:09 ` Tan, Jianfeng
2018-03-31 17:06 ` [dpdk-dev] [PATCH v7 3/3] eal: add asynchronous request API to DPDK IPC Anatoly Burakov
2018-04-04 22:15 ` Thomas Monjalon
2018-03-27 13:59 ` [dpdk-dev] [PATCH v6 2/2] " Anatoly Burakov
2018-03-27 16:33 ` Thomas Monjalon
2018-03-28 2:08 ` Tan, Jianfeng
2018-03-28 7:29 ` Thomas Monjalon
2018-03-28 8:22 ` Van Haaren, Harry
2018-03-28 8:55 ` Tan, Jianfeng
2018-03-28 9:10 ` Van Haaren, Harry
2018-03-28 9:21 ` Burakov, Anatoly
2018-03-28 9:53 ` Thomas Monjalon
2018-03-28 10:42 ` Burakov, Anatoly [this message]
2018-03-28 11:26 ` Thomas Monjalon
2018-03-28 12:21 ` Burakov, Anatoly
2018-03-28 9:11 ` Bruce Richardson
2018-03-24 12:46 ` [dpdk-dev] [PATCH v5 " Anatoly Burakov
2018-03-26 14:15 ` Tan, Jianfeng
2018-03-26 14:28 ` Burakov, Anatoly
2018-03-02 18:48 ` [dpdk-dev] [PATCH] " Stephen Hemminger
2018-03-03 12:29 ` Burakov, Anatoly
2018-03-02 18:51 ` Stephen Hemminger
2018-03-03 13:44 ` Burakov, Anatoly
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=5a7e8c12-9336-678a-ccdb-4938bb64e9c2@intel.com \
--to=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=harry.van.haaren@intel.com \
--cc=jianfeng.tan@intel.com \
--cc=konstantin.ananyev@intel.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).