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:tho...@monjalon.net]
28/03/2018 04:08, Tan, Jianfeng:
From: Thomas Monjalon [mailto:tho...@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
