[v4,06/24] ethdev: enable hotplug on multi-process

  We are going to introduce the solution to handle different hotplug
cases in multi-process situation, it include below scenario:

1. Attach a share device from primary
2. Detach a share device from primary
3. Attach a share device from secondary
4. Detach a share device from secondary
5. Attach a private device from secondary
6. Detach a private device from secondary
7. Detach a share device from secondary privately
8. Attach a share device from secondary privately

In primary-secondary process model, we assume device is shared by default.
that means attach or detach a device on any process will broadcast to
all other processes through mp channel then device information will be
synchronized on all processes.

Any failure during attaching process will cause inconsistent status
between processes, so proper rollback action should be considered.
Also it is not safe to detach a share device when other process still use
it, so a handshake mechanism is introduced.

This patch covers the implementation of case 1,2,5,6,7,8.
Case 3,4 will be implemented on separate patch as well as handshake
mechanism.

Scenario for Case 1, 2:

attach device
a) primary attach the new device if failed goto h).
b) primary send attach sync request to all secondary.
c) secondary receive request and attach device and send reply.
d) primary check the reply if all success go to i).
e) primary send attach rollback sync request to all secondary.
f) secondary receive the request and detach device and send reply.
g) primary receive the reply and detach device as rollback action.
h) attach fail
i) attach success

detach device
a) primary perform pre-detach check, if device is locked, goto i).
b) primary send pre-detach sync request to all secondary.
c) secondary perform pre-detach check and send reply.
d) primary check the reply if any fail goto i).
e) primary send detach sync request to all secondary
f) secondary detach the device and send reply (assume no fail)
g) primary detach the device.
h) detach success
i) detach failed

Case 5, 6:
Secondary process can attach private device which only visible to itself,
in this case no IPC is involved, primary process is not allowed to have
private device so far.

Case 7, 8:
Secondary process can also temporally to detach a share device "privately"
then attach it back later, this action also not impact other processes.

APIs changes:

rte_eth_dev_attach and rte_eth_dev_attach are extended to support
share device attach/detach in primary-secondary process model, it will
be called in case 1,2,3,4.

New API rte_eth_dev_attach_private and rte_eth_dev_detach_private are
introduced to cover case 5,6,7,8, this API can only be invoked in secondary
process.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_ethdev/Makefile          |   1 +
 lib/librte_ethdev/ethdev_mp.c       | 248 ++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/ethdev_mp.h       |  41 ++++++
 lib/librte_ethdev/ethdev_private.h  |  39 ++++++
 lib/librte_ethdev/meson.build       |   1 +
 lib/librte_ethdev/rte_ethdev.c      | 190 ++++++++++++++++++++++++---
 lib/librte_ethdev/rte_ethdev.h      |  45 +++++++
 lib/librte_ethdev/rte_ethdev_core.h |   5 +
 8 files changed, 553 insertions(+), 17 deletions(-)
 create mode 100644 lib/librte_ethdev/ethdev_mp.c
 create mode 100644 lib/librte_ethdev/ethdev_mp.h
 create mode 100644 lib/librte_ethdev/ethdev_private.h
  

On 26-Jun-18 8:08 AM, Qi Zhang wrote:
> We are going to introduce the solution to handle different hotplug
> cases in multi-process situation, it include below scenario:
> 
> 1. Attach a share device from primary
> 2. Detach a share device from primary
> 3. Attach a share device from secondary
> 4. Detach a share device from secondary
> 5. Attach a private device from secondary
> 6. Detach a private device from secondary
> 7. Detach a share device from secondary privately
> 8. Attach a share device from secondary privately
> 
> In primary-secondary process model, we assume device is shared by default.
> that means attach or detach a device on any process will broadcast to
> all other processes through mp channel then device information will be
> synchronized on all processes.
> 
> Any failure during attaching process will cause inconsistent status
> between processes, so proper rollback action should be considered.
> Also it is not safe to detach a share device when other process still use
> it, so a handshake mechanism is introduced.
> 
> This patch covers the implementation of case 1,2,5,6,7,8.
> Case 3,4 will be implemented on separate patch as well as handshake
> mechanism.
> 
> Scenario for Case 1, 2:
> 
> attach device
> a) primary attach the new device if failed goto h).
> b) primary send attach sync request to all secondary.
> c) secondary receive request and attach device and send reply.
> d) primary check the reply if all success go to i).
> e) primary send attach rollback sync request to all secondary.
> f) secondary receive the request and detach device and send reply.
> g) primary receive the reply and detach device as rollback action.
> h) attach fail
> i) attach success
> 
> detach device
> a) primary perform pre-detach check, if device is locked, goto i).
> b) primary send pre-detach sync request to all secondary.
> c) secondary perform pre-detach check and send reply.
> d) primary check the reply if any fail goto i).
> e) primary send detach sync request to all secondary
> f) secondary detach the device and send reply (assume no fail)
> g) primary detach the device.
> h) detach success
> i) detach failed
> 
> Case 5, 6:
> Secondary process can attach private device which only visible to itself,
> in this case no IPC is involved, primary process is not allowed to have
> private device so far.
> 
> Case 7, 8:
> Secondary process can also temporally to detach a share device "privately"
> then attach it back later, this action also not impact other processes.
> 
> APIs changes:
> 
> rte_eth_dev_attach and rte_eth_dev_attach are extended to support
> share device attach/detach in primary-secondary process model, it will
> be called in case 1,2,3,4.
> 
> New API rte_eth_dev_attach_private and rte_eth_dev_detach_private are
> introduced to cover case 5,6,7,8, this API can only be invoked in secondary
> process.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---

<snip>

> +static int
> +handle_primary_request(const struct rte_mp_msg *msg, const void *peer)
> +{
> +
> +	struct rte_mp_msg mp_resp;
> +	const struct eth_dev_mp_req *req =
> +		(const struct eth_dev_mp_req *)msg->param;
> +	struct eth_dev_mp_req *resp =
> +		(struct eth_dev_mp_req *)mp_resp.param;
> +	struct mp_reply_bundle *bundle;
> +	int ret = 0;
> +
> +	memset(&mp_resp, 0, sizeof(mp_resp));
> +	strlcpy(mp_resp.name, ETH_DEV_MP_ACTION_REQUEST, sizeof(mp_resp.name));
> +	mp_resp.len_param = sizeof(*req);
> +	memcpy(resp, req, sizeof(*resp));
> +
> +	bundle = calloc(1, sizeof(*bundle));
> +	if (bundle == NULL) {
> +		resp->result = -ENOMEM;
> +		ret = rte_mp_reply(&mp_resp, peer);
> +		if (ret) {
> +			ethdev_log(ERR, "failed to send reply to primary request\n");
> +			return ret;
> +		}
> +	}
> +
> +	bundle->msg = *msg;
> +	bundle->peer = peer;
> +
> +	ret = rte_eal_mp_task_add(__handle_primary_request, bundle);
> +	if (ret) {
> +		resp->result = ret;
> +		ret = rte_mp_reply(&mp_resp, peer);
> +		if (ret) {
> +			ethdev_log(ERR, "failed to send reply to primary request\n");
> +			return ret;
> +		}
> +	}

What you're doing here is quite dangerous. The parameter "const void 
*peer" is only guaranteed to be valid at the time of the callback - not 
necessarily afterwards. So, if you're handing off sending replies to a 
separate thread, things might blow up because the pointer may no longer 
be valid.
  

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 8:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> 
> On 26-Jun-18 8:08 AM, Qi Zhang wrote:
> > We are going to introduce the solution to handle different hotplug
> > cases in multi-process situation, it include below scenario:
> >
> > 1. Attach a share device from primary
> > 2. Detach a share device from primary
> > 3. Attach a share device from secondary 4. Detach a share device from
> > secondary 5. Attach a private device from secondary 6. Detach a
> > private device from secondary 7. Detach a share device from secondary
> > privately 8. Attach a share device from secondary privately
> >
> > In primary-secondary process model, we assume device is shared by default.
> > that means attach or detach a device on any process will broadcast to
> > all other processes through mp channel then device information will be
> > synchronized on all processes.
> >
> > Any failure during attaching process will cause inconsistent status
> > between processes, so proper rollback action should be considered.
> > Also it is not safe to detach a share device when other process still
> > use it, so a handshake mechanism is introduced.
> >
> > This patch covers the implementation of case 1,2,5,6,7,8.
> > Case 3,4 will be implemented on separate patch as well as handshake
> > mechanism.
> >
> > Scenario for Case 1, 2:
> >
> > attach device
> > a) primary attach the new device if failed goto h).
> > b) primary send attach sync request to all secondary.
> > c) secondary receive request and attach device and send reply.
> > d) primary check the reply if all success go to i).
> > e) primary send attach rollback sync request to all secondary.
> > f) secondary receive the request and detach device and send reply.
> > g) primary receive the reply and detach device as rollback action.
> > h) attach fail
> > i) attach success
> >
> > detach device
> > a) primary perform pre-detach check, if device is locked, goto i).
> > b) primary send pre-detach sync request to all secondary.
> > c) secondary perform pre-detach check and send reply.
> > d) primary check the reply if any fail goto i).
> > e) primary send detach sync request to all secondary
> > f) secondary detach the device and send reply (assume no fail)
> > g) primary detach the device.
> > h) detach success
> > i) detach failed
> >
> > Case 5, 6:
> > Secondary process can attach private device which only visible to
> > itself, in this case no IPC is involved, primary process is not
> > allowed to have private device so far.
> >
> > Case 7, 8:
> > Secondary process can also temporally to detach a share device "privately"
> > then attach it back later, this action also not impact other processes.
> >
> > APIs changes:
> >
> > rte_eth_dev_attach and rte_eth_dev_attach are extended to support
> > share device attach/detach in primary-secondary process model, it will
> > be called in case 1,2,3,4.
> >
> > New API rte_eth_dev_attach_private and rte_eth_dev_detach_private are
> > introduced to cover case 5,6,7,8, this API can only be invoked in
> > secondary process.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> 
> <snip>
> 
> > +static int
> > +handle_primary_request(const struct rte_mp_msg *msg, const void
> > +*peer) {
> > +
> > +	struct rte_mp_msg mp_resp;
> > +	const struct eth_dev_mp_req *req =
> > +		(const struct eth_dev_mp_req *)msg->param;
> > +	struct eth_dev_mp_req *resp =
> > +		(struct eth_dev_mp_req *)mp_resp.param;
> > +	struct mp_reply_bundle *bundle;
> > +	int ret = 0;
> > +
> > +	memset(&mp_resp, 0, sizeof(mp_resp));
> > +	strlcpy(mp_resp.name, ETH_DEV_MP_ACTION_REQUEST,
> sizeof(mp_resp.name));
> > +	mp_resp.len_param = sizeof(*req);
> > +	memcpy(resp, req, sizeof(*resp));
> > +
> > +	bundle = calloc(1, sizeof(*bundle));
> > +	if (bundle == NULL) {
> > +		resp->result = -ENOMEM;
> > +		ret = rte_mp_reply(&mp_resp, peer);
> > +		if (ret) {
> > +			ethdev_log(ERR, "failed to send reply to primary request\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	bundle->msg = *msg;
> > +	bundle->peer = peer;
> > +
> > +	ret = rte_eal_mp_task_add(__handle_primary_request, bundle);
> > +	if (ret) {
> > +		resp->result = ret;
> > +		ret = rte_mp_reply(&mp_resp, peer);
> > +		if (ret) {
> > +			ethdev_log(ERR, "failed to send reply to primary request\n");
> > +			return ret;
> > +		}
> > +	}
> 
> What you're doing here is quite dangerous. The parameter "const void *peer"
> is only guaranteed to be valid at the time of the callback - not necessarily
> afterwards. So, if you're handing off sending replies to a separate thread,
> things might blow up because the pointer may no longer be valid.

OK, so what about clone the content a buffer, I think the content should be valid before reply is sent, right?

Thanks
Qi
> 
> --
> Thanks,
> Anatoly
  

On 26-Jun-18 1:19 PM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Tuesday, June 26, 2018 8:09 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
>> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Shelton, Benjamin H
>> <benjamin.h.shelton@intel.com>; Vangati, Narender
>> <narender.vangati@intel.com>
>> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
>>
>> On 26-Jun-18 8:08 AM, Qi Zhang wrote:
>>> We are going to introduce the solution to handle different hotplug
>>> cases in multi-process situation, it include below scenario:
>>>
>>> 1. Attach a share device from primary
>>> 2. Detach a share device from primary
>>> 3. Attach a share device from secondary 4. Detach a share device from
>>> secondary 5. Attach a private device from secondary 6. Detach a
>>> private device from secondary 7. Detach a share device from secondary
>>> privately 8. Attach a share device from secondary privately
>>>
>>> In primary-secondary process model, we assume device is shared by default.
>>> that means attach or detach a device on any process will broadcast to
>>> all other processes through mp channel then device information will be
>>> synchronized on all processes.
>>>
>>> Any failure during attaching process will cause inconsistent status
>>> between processes, so proper rollback action should be considered.
>>> Also it is not safe to detach a share device when other process still
>>> use it, so a handshake mechanism is introduced.
>>>
>>> This patch covers the implementation of case 1,2,5,6,7,8.
>>> Case 3,4 will be implemented on separate patch as well as handshake
>>> mechanism.
>>>
>>> Scenario for Case 1, 2:
>>>
>>> attach device
>>> a) primary attach the new device if failed goto h).
>>> b) primary send attach sync request to all secondary.
>>> c) secondary receive request and attach device and send reply.
>>> d) primary check the reply if all success go to i).
>>> e) primary send attach rollback sync request to all secondary.
>>> f) secondary receive the request and detach device and send reply.
>>> g) primary receive the reply and detach device as rollback action.
>>> h) attach fail
>>> i) attach success
>>>
>>> detach device
>>> a) primary perform pre-detach check, if device is locked, goto i).
>>> b) primary send pre-detach sync request to all secondary.
>>> c) secondary perform pre-detach check and send reply.
>>> d) primary check the reply if any fail goto i).
>>> e) primary send detach sync request to all secondary
>>> f) secondary detach the device and send reply (assume no fail)
>>> g) primary detach the device.
>>> h) detach success
>>> i) detach failed
>>>
>>> Case 5, 6:
>>> Secondary process can attach private device which only visible to
>>> itself, in this case no IPC is involved, primary process is not
>>> allowed to have private device so far.
>>>
>>> Case 7, 8:
>>> Secondary process can also temporally to detach a share device "privately"
>>> then attach it back later, this action also not impact other processes.
>>>
>>> APIs changes:
>>>
>>> rte_eth_dev_attach and rte_eth_dev_attach are extended to support
>>> share device attach/detach in primary-secondary process model, it will
>>> be called in case 1,2,3,4.
>>>
>>> New API rte_eth_dev_attach_private and rte_eth_dev_detach_private are
>>> introduced to cover case 5,6,7,8, this API can only be invoked in
>>> secondary process.
>>>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>> ---
>>
>> <snip>
>>
>>> +static int
>>> +handle_primary_request(const struct rte_mp_msg *msg, const void
>>> +*peer) {
>>> +
>>> +	struct rte_mp_msg mp_resp;
>>> +	const struct eth_dev_mp_req *req =
>>> +		(const struct eth_dev_mp_req *)msg->param;
>>> +	struct eth_dev_mp_req *resp =
>>> +		(struct eth_dev_mp_req *)mp_resp.param;
>>> +	struct mp_reply_bundle *bundle;
>>> +	int ret = 0;
>>> +
>>> +	memset(&mp_resp, 0, sizeof(mp_resp));
>>> +	strlcpy(mp_resp.name, ETH_DEV_MP_ACTION_REQUEST,
>> sizeof(mp_resp.name));
>>> +	mp_resp.len_param = sizeof(*req);
>>> +	memcpy(resp, req, sizeof(*resp));
>>> +
>>> +	bundle = calloc(1, sizeof(*bundle));
>>> +	if (bundle == NULL) {
>>> +		resp->result = -ENOMEM;
>>> +		ret = rte_mp_reply(&mp_resp, peer);
>>> +		if (ret) {
>>> +			ethdev_log(ERR, "failed to send reply to primary request\n");
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	bundle->msg = *msg;
>>> +	bundle->peer = peer;
>>> +
>>> +	ret = rte_eal_mp_task_add(__handle_primary_request, bundle);
>>> +	if (ret) {
>>> +		resp->result = ret;
>>> +		ret = rte_mp_reply(&mp_resp, peer);
>>> +		if (ret) {
>>> +			ethdev_log(ERR, "failed to send reply to primary request\n");
>>> +			return ret;
>>> +		}
>>> +	}
>>
>> What you're doing here is quite dangerous. The parameter "const void *peer"
>> is only guaranteed to be valid at the time of the callback - not necessarily
>> afterwards. So, if you're handing off sending replies to a separate thread,
>> things might blow up because the pointer may no longer be valid.
> 
> OK, so what about clone the content a buffer, I think the content should be valid before reply is sent, right?

Yes, but even if you clone the content of the buffer, where would you 
send it *to*? You'll need the peer parameter to know where to send your 
response.

> 
> Thanks
> Qi
>>
>> --
>> Thanks,
>> Anatoly
  

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 8:50 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> 
> On 26-Jun-18 1:19 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly
> >> Sent: Tuesday, June 26, 2018 8:09 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> >> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> >> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> >> <benjamin.h.shelton@intel.com>; Vangati, Narender
> >> <narender.vangati@intel.com>
> >> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> >>
> >> On 26-Jun-18 8:08 AM, Qi Zhang wrote:
> >>> We are going to introduce the solution to handle different hotplug
> >>> cases in multi-process situation, it include below scenario:
> >>>
> >>> 1. Attach a share device from primary 2. Detach a share device from
> >>> primary 3. Attach a share device from secondary 4. Detach a share
> >>> device from secondary 5. Attach a private device from secondary 6.
> >>> Detach a private device from secondary 7. Detach a share device from
> >>> secondary privately 8. Attach a share device from secondary
> >>> privately
> >>>
> >>> In primary-secondary process model, we assume device is shared by
> default.
> >>> that means attach or detach a device on any process will broadcast
> >>> to all other processes through mp channel then device information
> >>> will be synchronized on all processes.
> >>>
> >>> Any failure during attaching process will cause inconsistent status
> >>> between processes, so proper rollback action should be considered.
> >>> Also it is not safe to detach a share device when other process
> >>> still use it, so a handshake mechanism is introduced.
> >>>
> >>> This patch covers the implementation of case 1,2,5,6,7,8.
> >>> Case 3,4 will be implemented on separate patch as well as handshake
> >>> mechanism.
> >>>
> >>> Scenario for Case 1, 2:
> >>>
> >>> attach device
> >>> a) primary attach the new device if failed goto h).
> >>> b) primary send attach sync request to all secondary.
> >>> c) secondary receive request and attach device and send reply.
> >>> d) primary check the reply if all success go to i).
> >>> e) primary send attach rollback sync request to all secondary.
> >>> f) secondary receive the request and detach device and send reply.
> >>> g) primary receive the reply and detach device as rollback action.
> >>> h) attach fail
> >>> i) attach success
> >>>
> >>> detach device
> >>> a) primary perform pre-detach check, if device is locked, goto i).
> >>> b) primary send pre-detach sync request to all secondary.
> >>> c) secondary perform pre-detach check and send reply.
> >>> d) primary check the reply if any fail goto i).
> >>> e) primary send detach sync request to all secondary
> >>> f) secondary detach the device and send reply (assume no fail)
> >>> g) primary detach the device.
> >>> h) detach success
> >>> i) detach failed
> >>>
> >>> Case 5, 6:
> >>> Secondary process can attach private device which only visible to
> >>> itself, in this case no IPC is involved, primary process is not
> >>> allowed to have private device so far.
> >>>
> >>> Case 7, 8:
> >>> Secondary process can also temporally to detach a share device "privately"
> >>> then attach it back later, this action also not impact other processes.
> >>>
> >>> APIs changes:
> >>>
> >>> rte_eth_dev_attach and rte_eth_dev_attach are extended to support
> >>> share device attach/detach in primary-secondary process model, it
> >>> will be called in case 1,2,3,4.
> >>>
> >>> New API rte_eth_dev_attach_private and rte_eth_dev_detach_private
> >>> are introduced to cover case 5,6,7,8, this API can only be invoked
> >>> in secondary process.
> >>>
> >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>> ---
> >>
> >> <snip>
> >>
> >>> +static int
> >>> +handle_primary_request(const struct rte_mp_msg *msg, const void
> >>> +*peer) {
> >>> +
> >>> +	struct rte_mp_msg mp_resp;
> >>> +	const struct eth_dev_mp_req *req =
> >>> +		(const struct eth_dev_mp_req *)msg->param;
> >>> +	struct eth_dev_mp_req *resp =
> >>> +		(struct eth_dev_mp_req *)mp_resp.param;
> >>> +	struct mp_reply_bundle *bundle;
> >>> +	int ret = 0;
> >>> +
> >>> +	memset(&mp_resp, 0, sizeof(mp_resp));
> >>> +	strlcpy(mp_resp.name, ETH_DEV_MP_ACTION_REQUEST,
> >> sizeof(mp_resp.name));
> >>> +	mp_resp.len_param = sizeof(*req);
> >>> +	memcpy(resp, req, sizeof(*resp));
> >>> +
> >>> +	bundle = calloc(1, sizeof(*bundle));
> >>> +	if (bundle == NULL) {
> >>> +		resp->result = -ENOMEM;
> >>> +		ret = rte_mp_reply(&mp_resp, peer);
> >>> +		if (ret) {
> >>> +			ethdev_log(ERR, "failed to send reply to primary
> request\n");
> >>> +			return ret;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	bundle->msg = *msg;
> >>> +	bundle->peer = peer;
> >>> +
> >>> +	ret = rte_eal_mp_task_add(__handle_primary_request, bundle);
> >>> +	if (ret) {
> >>> +		resp->result = ret;
> >>> +		ret = rte_mp_reply(&mp_resp, peer);
> >>> +		if (ret) {
> >>> +			ethdev_log(ERR, "failed to send reply to primary
> request\n");
> >>> +			return ret;
> >>> +		}
> >>> +	}
> >>
> >> What you're doing here is quite dangerous. The parameter "const void
> *peer"
> >> is only guaranteed to be valid at the time of the callback - not
> >> necessarily afterwards. So, if you're handing off sending replies to
> >> a separate thread, things might blow up because the pointer may no longer
> be valid.
> >
> > OK, so what about clone the content a buffer, I think the content should be
> valid before reply is sent, right?
> 
> Yes, but even if you clone the content of the buffer, where would you send it
> *to*? You'll need the peer parameter to know where to send your response.

my understand is peer is identified by a string (or filename)
what I mean is clone the content of the buffer that peer point to , 
So I don't need to worry if the original peer be used to point to some other data



> 
> >
> > Thanks
> > Qi
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly
  

On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
> 
> my understand is peer is identified by a string (or filename)
> what I mean is clone the content of the buffer that peer point to ,
> So I don't need to worry if the original peer be used to point to some other data
> 

As far as the application is concerned, peer is an opaque pointer, and 
should be treated as such. Peeking behind a void pointer that is not 
designed for this purpose is not a good idea, even if technically you 
know what's in there.
  

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 9:21 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> 
> On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
> >
> > my understand is peer is identified by a string (or filename) what I
> > mean is clone the content of the buffer that peer point to , So I
> > don't need to worry if the original peer be used to point to some
> > other data
> >
> 
> As far as the application is concerned, peer is an opaque pointer, and should
> be treated as such. Peeking behind a void pointer that is not designed for this
> purpose is not a good idea, even if technically you know what's in there.

We can expose a clone interface, like MP_PEER_CLONE, so we don't need to know what's inside, just need to know that it can be used on another thread?


> 
> --
> Thanks,
> Anatoly
  

On 26-Jun-18 2:25 PM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Tuesday, June 26, 2018 9:21 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
>> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Shelton, Benjamin H
>> <benjamin.h.shelton@intel.com>; Vangati, Narender
>> <narender.vangati@intel.com>
>> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
>>
>> On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
>>>
>>> my understand is peer is identified by a string (or filename) what I
>>> mean is clone the content of the buffer that peer point to , So I
>>> don't need to worry if the original peer be used to point to some
>>> other data
>>>
>>
>> As far as the application is concerned, peer is an opaque pointer, and should
>> be treated as such. Peeking behind a void pointer that is not designed for this
>> purpose is not a good idea, even if technically you know what's in there.
> 
> We can expose a clone interface, like MP_PEER_CLONE, so we don't need to know what's inside, just need to know that it can be used on another thread?
> 

Well, that can probably work. Feels like a hacky workaround though.

Another way to do the same thing would be to store peer information 
right in the message, as opposed to providing it separately. Still a 
hack though, and will require far more changes, but it could be a better 
solution as (if done right) it would allow identifying which reply came 
from which peer.

Of course, an even better approach would be to devise some kind of 
addressing scheme (uuid?), so that peer addresses are no longer opaque 
pointers but rather are valid data types.

Thoughts?
  

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 9:46 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> 
> On 26-Jun-18 2:25 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly
> >> Sent: Tuesday, June 26, 2018 9:21 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> >> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> >> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> >> <benjamin.h.shelton@intel.com>; Vangati, Narender
> >> <narender.vangati@intel.com>
> >> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> >>
> >> On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
> >>>
> >>> my understand is peer is identified by a string (or filename) what I
> >>> mean is clone the content of the buffer that peer point to , So I
> >>> don't need to worry if the original peer be used to point to some
> >>> other data
> >>>
> >>
> >> As far as the application is concerned, peer is an opaque pointer,
> >> and should be treated as such. Peeking behind a void pointer that is
> >> not designed for this purpose is not a good idea, even if technically you
> know what's in there.
> >
> > We can expose a clone interface, like MP_PEER_CLONE, so we don't need to
> know what's inside, just need to know that it can be used on another thread?
> >
> 
> Well, that can probably work. Feels like a hacky workaround though.
> 
> Another way to do the same thing would be to store peer information right in
> the message, as opposed to providing it separately. Still a hack though, and will
> require far more changes, but it could be a better solution as (if done right) it
> would allow identifying which reply came from which peer.
> 
> Of course, an even better approach would be to devise some kind of
> addressing scheme (uuid?), so that peer addresses are no longer opaque
> pointers but rather are valid data types.
> 
> Thoughts?

I may not give insight comment from the IPC implementation, but from user's view, what required is a unique token,
 it can be used for reply at anywhere and at any time, to me, currently implementation looks like missing some mapping management between an abstract token to its real data.  

Thanks
Qi.

> 
> --
> Thanks,
> Anatoly
  

On 26-Jun-18 3:24 PM, Zhang, Qi Z wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Tuesday, June 26, 2018 9:46 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
>> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Shelton, Benjamin H
>> <benjamin.h.shelton@intel.com>; Vangati, Narender
>> <narender.vangati@intel.com>
>> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
>>
>> On 26-Jun-18 2:25 PM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Burakov, Anatoly
>>>> Sent: Tuesday, June 26, 2018 9:21 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
>>>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
>>>> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>; Shelton, Benjamin H
>>>> <benjamin.h.shelton@intel.com>; Vangati, Narender
>>>> <narender.vangati@intel.com>
>>>> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
>>>>
>>>> On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
>>>>>
>>>>> my understand is peer is identified by a string (or filename) what I
>>>>> mean is clone the content of the buffer that peer point to , So I
>>>>> don't need to worry if the original peer be used to point to some
>>>>> other data
>>>>>
>>>>
>>>> As far as the application is concerned, peer is an opaque pointer,
>>>> and should be treated as such. Peeking behind a void pointer that is
>>>> not designed for this purpose is not a good idea, even if technically you
>> know what's in there.
>>>
>>> We can expose a clone interface, like MP_PEER_CLONE, so we don't need to
>> know what's inside, just need to know that it can be used on another thread?
>>>
>>
>> Well, that can probably work. Feels like a hacky workaround though.
>>
>> Another way to do the same thing would be to store peer information right in
>> the message, as opposed to providing it separately. Still a hack though, and will
>> require far more changes, but it could be a better solution as (if done right) it
>> would allow identifying which reply came from which peer.
>>
>> Of course, an even better approach would be to devise some kind of
>> addressing scheme (uuid?), so that peer addresses are no longer opaque
>> pointers but rather are valid data types.
>>
>> Thoughts?
> 
> I may not give insight comment from the IPC implementation, but from user's view, what required is a unique token,
>   it can be used for reply at anywhere and at any time, to me, currently implementation looks like missing some mapping management between an abstract token to its real data.
> 

Well, the idea was to not provide that :) We wanted to keep it simple 
and actively discourage any attempts to know which peer you're 
communicating with, as allowing that would imply some kind of addressing 
scheme, peer discovery and things like that, which we don't want to 
bother with. If you want replies, you use callbacks, which provide you 
with a peer address.

On the one hand, i understand the frustration of being forced to deal 
with deliberate simplicity of IPC API and to create workarounds like 
doing sendmsg() and storing requests that you're waiting on inside the 
application and launching interrupt threads, all for things that should 
Just Work in an IPC API. Believe me, i know about its deficiencies more 
than most people :)

On the other hand, we don't want to introduce hacks to solve a specific 
problem. If we are to solve this in IPC, we should do it properly. I 
don't think adding a workaround with "cloning" peer address is the 
solution. A proper solution would've been to implement addressing 
scheme, so that any response could be submitted at any time.

The hard route would be to add peer discovery, etc. The easy route would 
be to just change the peer address to something typed, something that we 
can use to reconstruct the original peer id. A great candidate for this 
is the uuid, but it's an external dependency. A workaround for this 
would be to just use a uint64 value obtained through some magic that is 
part of socket path. For example, replace 
"/var/run/dpdk/rte/mp_socket_pid_rdtsc" with something just as (likely) 
unique, but something that can be put into a uint64.

Without significantly changing the API and the internals, i think this 
is the best we will be able to do.

> Thanks
> Qi.
> 
>>
>> --
>> Thanks,
>> Anatoly
  

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 11:12 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> 
> On 26-Jun-18 3:24 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly
> >> Sent: Tuesday, June 26, 2018 9:46 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> >> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> >> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> >> <benjamin.h.shelton@intel.com>; Vangati, Narender
> >> <narender.vangati@intel.com>
> >> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on multi-process
> >>
> >> On 26-Jun-18 2:25 PM, Zhang, Qi Z wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Burakov, Anatoly
> >>>> Sent: Tuesday, June 26, 2018 9:21 PM
> >>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> >>>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> >>>> dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> >>>> Yigit, Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H
> >>>> <benjamin.h.shelton@intel.com>; Vangati, Narender
> >>>> <narender.vangati@intel.com>
> >>>> Subject: Re: [PATCH v4 06/24] ethdev: enable hotplug on
> >>>> multi-process
> >>>>
> >>>> On 26-Jun-18 1:58 PM, Zhang, Qi Z wrote:
> >>>>>
> >>>>> my understand is peer is identified by a string (or filename) what
> >>>>> I mean is clone the content of the buffer that peer point to , So
> >>>>> I don't need to worry if the original peer be used to point to
> >>>>> some other data
> >>>>>
> >>>>
> >>>> As far as the application is concerned, peer is an opaque pointer,
> >>>> and should be treated as such. Peeking behind a void pointer that
> >>>> is not designed for this purpose is not a good idea, even if
> >>>> technically you
> >> know what's in there.
> >>>
> >>> We can expose a clone interface, like MP_PEER_CLONE, so we don't
> >>> need to
> >> know what's inside, just need to know that it can be used on another
> thread?
> >>>
> >>
> >> Well, that can probably work. Feels like a hacky workaround though.
> >>
> >> Another way to do the same thing would be to store peer information
> >> right in the message, as opposed to providing it separately. Still a
> >> hack though, and will require far more changes, but it could be a
> >> better solution as (if done right) it would allow identifying which reply came
> from which peer.
> >>
> >> Of course, an even better approach would be to devise some kind of
> >> addressing scheme (uuid?), so that peer addresses are no longer
> >> opaque pointers but rather are valid data types.
> >>
> >> Thoughts?
> >
> > I may not give insight comment from the IPC implementation, but from
> user's view, what required is a unique token,
> >   it can be used for reply at anywhere and at any time, to me, currently
> implementation looks like missing some mapping management between an
> abstract token to its real data.
> >
> 
> Well, the idea was to not provide that :) We wanted to keep it simple and
> actively discourage any attempts to know which peer you're communicating
> with, as allowing that would imply some kind of addressing scheme, peer
> discovery and things like that, which we don't want to bother with. If you want
> replies, you use callbacks, which provide you with a peer address.
> 
> On the one hand, i understand the frustration of being forced to deal with
> deliberate simplicity of IPC API and to create workarounds like doing sendmsg()
> and storing requests that you're waiting on inside the application and
> launching interrupt threads, all for things that should Just Work in an IPC API.
> Believe me, i know about its deficiencies more than most people :)
> 
> On the other hand, we don't want to introduce hacks to solve a specific
> problem. If we are to solve this in IPC, we should do it properly. I don't think
> adding a workaround with "cloning" peer address is the solution. A proper
> solution would've been to implement addressing scheme, so that any
> response could be submitted at any time.
> 
> The hard route would be to add peer discovery, etc. The easy route would be
> to just change the peer address to something typed, something that we can
> use to reconstruct the original peer id. A great candidate for this is the uuid,
> but it's an external dependency. A workaround for this would be to just use a
> uint64 value obtained through some magic that is part of socket path. For
> example, replace "/var/run/dpdk/rte/mp_socket_pid_rdtsc" with something
> just as (likely) unique, but something that can be put into a uint64.
> 
> Without significantly changing the API and the internals, i think this is the best
> we will be able to do.

Ok, let me summarize this.

Current sync IPC does not support reply to a sync request outside the callback function, 
Of cause it will not support reply on separate thread which required by mp hotplug implementation.

So, the question is do we agree that IPC should support a more relax sync reply? 
For my option, I think the requirement is reasonable, because it greatly simplified the implementation of the case when the receiver need to do some task on a separate thread and reply on that thread.(note, this is typical case since the IPC deadlock limitation)

If the answer is yes:

	The question is will we able to enable this in this release?
	For me I think I may not able to solve this quickly, I need more time to review the implementation detail of IPC , not sure if you can cover this on your new patchset for IPC. 
	if yes, I assume I don't need to change any code (or a little bit) of current mp implementation, 	
	because "peer"(or whatever) as an identity of reply target can be parsed to other thread

	If not, we can take workaround for mp hotplug, 
		since it is workaround I prefer the simplest way with less impact and enough comments, since we will fix this anyway.
		Yes, clone is the simplest way :), I can just implement it inside ethdev_mp.c and add comment say "Fix me, this is hack"., so it will not impact other IPC users.

If the answer is NO
	I have to give up all sync IPC, and use sendmsg to replace sync request and to match request / reply in ethdev_mp.c , though it looks like should be a part of IPC and this looks like we are going to enable another set of sync IPC which support reply on another thread.

Thanks
Qi
> 
> > Thanks
> > Qi.
> >
> >>
> >> --
> >> Thanks,
> >> Anatoly
> 
> 
> --
> Thanks,
> Anatoly