[dpdk-dev,v6,2/2] eal: add asynchronous request API to DPDK IPC

  This API is similar to the blocking API that is already present,
but reply will be received in a separate callback by the caller
(callback specified at the time of request, rather than registering
for it in advance).

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.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>
---

Notes:
    v6:
      - address review comments from Jianfeng
      - do not add dummy item to queue by default
    
    v5:
      - addressed review comments from Jianfeng
      - split into two patches to avoid rename noise
      - do not mark ignored message as processed
    v4:
      - rebase on top of latest IPC Improvements patchset [2]
    
    v3:
      - added support for MP_IGN messages introduced in
        IPC improvements v5 patchset
    v2:
      - fixed deadlocks and race conditions by not calling
        callbacks while iterating over sync request list
      - fixed use-after-free by making a copy of request
      - changed API to also give user a copy of original
        request, so that they know to which message the
        callback is a reply to
      - fixed missing .map file entries
    
    This patch is dependent upon previously published patchsets
    for IPC fixes [1] and improvements [2].
    
    rte_mp_action_unregister and rte_mp_async_reply_unregister
    do the same thing - should we perhaps make it one function?
    
    [1] http://dpdk.org/dev/patchwork/bundle/aburakov/IPC_Fixes/
    [2] http://dpdk.org/dev/patchwork/bundle/aburakov/IPC_Improvements/

 lib/librte_eal/common/eal_common_proc.c | 458 +++++++++++++++++++++++++++++++-
 lib/librte_eal/common/include/rte_eal.h |  36 +++
 lib/librte_eal/rte_eal_version.map      |   1 +
 3 files changed, 482 insertions(+), 13 deletions(-)
  

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?


> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -225,6 +225,7 @@ EXPERIMENTAL {
>  	rte_mp_action_unregister;
>  	rte_mp_sendmsg;
>  	rte_mp_request;
> +	rte_mp_request_async;

So there is rte_mp_request and rte_mp_request_async?
You should rename rte_mp_request, I guess.

>  	rte_mp_reply;
>  	rte_service_attr_get;
>  	rte_service_attr_reset_all;
  

Hi Thomas ,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, March 28, 2018 12:34 AM
> To: Burakov, Anatoly
> Cc: dev@dpdk.org; Tan, Jianfeng; Ananyev, Konstantin; Van Haaren, Harry
> Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request API to
> DPDK IPC
> 
> 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.

For rte_service, it may be not a good idea to reply on it as it needs explicit API calls to setup.

> 
> 
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -225,6 +225,7 @@ EXPERIMENTAL {
> >  	rte_mp_action_unregister;
> >  	rte_mp_sendmsg;
> >  	rte_mp_request;
> > +	rte_mp_request_async;
> 
> So there is rte_mp_request and rte_mp_request_async?
> You should rename rte_mp_request, I guess.

+1.

Thanks,
Jianfeng

> 
> >  	rte_mp_reply;
> >  	rte_service_attr_get;
> >  	rte_service_attr_reset_all;
> 
> 
> 
>
  

28/03/2018 04:08, Tan, Jianfeng:
> Hi Thomas ,
> 
> 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.
  

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, March 28, 2018 8:30 AM
> To: Tan, Jianfeng <jianfeng.tan@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request API to
> DPDK IPC
> 
> 28/03/2018 04:08, Tan, Jianfeng:
> > Hi Thomas ,
> >
> > 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.

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 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.
  

Hi Thomas and Harry,


On 3/28/2018 4:22 PM, Van Haaren, Harry wrote:
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Wednesday, March 28, 2018 8:30 AM
>> To: Tan, Jianfeng <jianfeng.tan@intel.com>
>> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Ananyev,
>> Konstantin <konstantin.ananyev@intel.com>; Van Haaren, Harry
>> <harry.van.haaren@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request API to
>> DPDK IPC
>>
>> 28/03/2018 04:08, Tan, Jianfeng:
>>> Hi Thomas ,
>>>
>>> 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
  

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Wednesday, March 28, 2018 9:55 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; 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
> 
> Hi Thomas and Harry,

Hey,


> On 3/28/2018 4:22 PM, Van Haaren, Harry wrote:
> >> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> Sent: Wednesday, March 28, 2018 8:30 AM
> >> To: Tan, Jianfeng <jianfeng.tan@intel.com>
> >> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Ananyev,
> >> Konstantin <konstantin.ananyev@intel.com>; Van Haaren, Harry
> >> <harry.van.haaren@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request API
> to
> >> DPDK IPC
> >>
> >> 28/03/2018 04:08, Tan, Jianfeng:
> >>> Hi Thomas ,
> >>>
> >>> 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

I don't suggest moving everything into interrupt thread.

We need to ensure the interrupt thread (aka, link status interrupt thread) is not
busy for too long. Certain work may cause un-acceptable delay in the interrupt
thread, hence perhaps we should have 2 core DPDK threads:

1) EAL interrupt thread: handles only interrupts, so is ~always blocked on some FDs

2) EAL "core" thread: handles the IPC work here, and any other irregular work that
   can take some time to complete. Adding all core DPDK work to this thread enables
   us to refactor and create a scalable / coherent design.


> > 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.

To correct my statement just above - some method capable of blocking reads (as opposed to
busy polling). In my mind, this method *must* allow waiting on multiple FDs,
as to *require* only 1 thread. We could use more if it makes sense to do so.


> > 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.

I don't think that service is the correct hammer for this problem.

As to interrupt thread or creating a new thread, what makes more sense given
the current codebase? Is the current implementation an acceptable short-term
solution, that gets reworked to be more generic in future?
  

On Wed, Mar 28, 2018 at 09:29:35AM +0200, Thomas Monjalon wrote:
> 28/03/2018 04:08, Tan, Jianfeng:
> > Hi Thomas ,
> > 
> > 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.
> 
+1
I think it would be worthwhile adding an interrupt thread to BSD, and it
should not be a massive amount of work. Having a single interrupt thread
has a lot of benefits, I think.

/Bruce
  

On 28-Mar-18 9:55 AM, Tan, Jianfeng wrote:
> Hi Thomas and Harry,
> 
> 
> On 3/28/2018 4:22 PM, Van Haaren, Harry wrote:
>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>> Sent: Wednesday, March 28, 2018 8:30 AM
>>> To: Tan, Jianfeng <jianfeng.tan@intel.com>
>>> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Ananyev,
>>> Konstantin <konstantin.ananyev@intel.com>; Van Haaren, Harry
>>> <harry.van.haaren@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v6 2/2] eal: add asynchronous request 
>>> API to
>>> DPDK IPC
>>>
>>> 28/03/2018 04:08, Tan, Jianfeng:
>>>> Hi Thomas ,
>>>>
>>>> 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. 
We're going to be removing a few threads from EAL as it is because of 
IPC (Jianfeng has already submitted patches for those), 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.

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.
  

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?

> 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 :)

> 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?
  

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.
  

28/03/2018 12:42, Burakov, Anatoly:
> On 28-Mar-18 10:53 AM, Thomas Monjalon wrote:
> > 28/03/2018 11:21, Burakov, Anatoly:
> >> 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 :)

No, it's never too late to discuss.
Merging a patch does not prevent discussing it :)

> >> 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).

Actually, you had the feedback yourself from the beginning.
You decided to gave up with interrupt thread because its implementation
is not complete (and maybe far from perfect).
There are some communities where it is not acceptable to workaround
core issues because of timing issues. I think we accept it in DPDK,
but I continue to question it, in order to be sure that everybody is OK
with this kind of tradeoff.

> 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.

The main concern is API.
If all these changes are internal only, and does not involve any major
API change, then I guess it is OK to pospone them in next release.
  

On 28-Mar-18 12:26 PM, Thomas Monjalon wrote:
> 28/03/2018 12:42, Burakov, Anatoly:
>> On 28-Mar-18 10:53 AM, Thomas Monjalon wrote:
>>> 28/03/2018 11:21, Burakov, Anatoly:
>> 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).
> 
> Actually, you had the feedback yourself from the beginning.
> You decided to gave up with interrupt thread because its implementation
> is not complete (and maybe far from perfect).

That's not quite how i see it, but OK, suppose so.

> There are some communities where it is not acceptable to workaround
> core issues because of timing issues. I think we accept it in DPDK,
> but I continue to question it, in order to be sure that everybody is OK
> with this kind of tradeoff.

The way i see it, not all API's are equal; some are more important than 
others. This is a new, experimental API that is not core to any DPDK 
function - it's not used on any hotpaths nor is it even that demanding 
(the two threads will be sleeping 99.999% of the time anyway). I think 
we're allowed to experiment on it before settling on an implementation 
that satisfies everyone :)

>> 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.
> 
> The main concern is API.
> If all these changes are internal only, and does not involve any major
> API change, then I guess it is OK to pospone them in next release.
> 

Yes, all of this is/will be internal to DPDK IPC - no externally visible 
changes whatsoever.