[dpdk-dev,10/12] vhost: support to kick in secondary process
Checks
Commit Message
To support kick in secondary process, we propose callfd_pri and
kickfd_pri to store the value in primary process; and by a new
API, rte_vhost_set_vring_effective_fd(), we can set effective
callfd and kickfd which can be used by secondary process.
Note in this case, either primary process or the secondary process
can kick the frontend; that is, they cannot kick a vring at the
same time.
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
lib/librte_vhost/rte_vhost.h | 3 +++
lib/librte_vhost/vhost.c | 27 +++++++++++++++++++++++++--
lib/librte_vhost/vhost.h | 3 +++
lib/librte_vhost/vhost_user.c | 4 ++--
4 files changed, 33 insertions(+), 4 deletions(-)
Comments
Firstly, very sorry for so late review!
On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> To support kick in secondary process, we propose callfd_pri and
> kickfd_pri to store the value in primary process; and by a new
> API, rte_vhost_set_vring_effective_fd(), we can set effective
> callfd and kickfd which can be used by secondary process.
>
> Note in this case, either primary process or the secondary process
> can kick the frontend; that is, they cannot kick a vring at the
> same time.
Since only one can work, why not just overwriting the fd? Say, you
could introudce some APIs like "rte_vhost_set_vring_callfd", then
you don't need to introduce few more fields like "callfd_pri".
--yliu
On 9/21/2017 11:33 AM, Yuanhan Liu wrote:
> Firstly, very sorry for so late review!
That is understood.
>
> On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
>> To support kick in secondary process, we propose callfd_pri and
>> kickfd_pri to store the value in primary process; and by a new
>> API, rte_vhost_set_vring_effective_fd(), we can set effective
>> callfd and kickfd which can be used by secondary process.
>>
>> Note in this case, either primary process or the secondary process
>> can kick the frontend; that is, they cannot kick a vring at the
>> same time.
> Since only one can work, why not just overwriting the fd? Say, you
> could introudce some APIs like "rte_vhost_set_vring_callfd", then
> you don't need to introduce few more fields like "callfd_pri".
That cannot address the below case:
1. Primary starts;
2. Secondary one starts; (if we overwrite it without storing it in some
other fields)
3. Secondary one exits;
4. Secondary two starts. (primary cannot share the fd with this
secondary process now, as this fd does not mean anything to the primary
process)
Thanks,
Jianfeng
On Thu, Sep 21, 2017 at 03:04:39PM +0800, Tan, Jianfeng wrote:
> >On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> >>To support kick in secondary process, we propose callfd_pri and
> >>kickfd_pri to store the value in primary process; and by a new
> >>API, rte_vhost_set_vring_effective_fd(), we can set effective
> >>callfd and kickfd which can be used by secondary process.
> >>
> >>Note in this case, either primary process or the secondary process
> >>can kick the frontend; that is, they cannot kick a vring at the
> >>same time.
> >Since only one can work, why not just overwriting the fd? Say, you
> >could introudce some APIs like "rte_vhost_set_vring_callfd", then
> >you don't need to introduce few more fields like "callfd_pri".
>
> That cannot address the below case:
> 1. Primary starts;
> 2. Secondary one starts; (if we overwrite it without storing it in some
> other fields)
> 3. Secondary one exits;
> 4. Secondary two starts. (primary cannot share the fd with this secondary
> process now, as this fd does not mean anything to the primary process)
I was thinking that those fds will be retrieved by the primary process
once? So thsoe it got at beginning are still valid?
--yliu
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Thursday, September 21, 2017 5:18 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; mtetsuyah@gmail.com
> Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
>
> On Thu, Sep 21, 2017 at 03:04:39PM +0800, Tan, Jianfeng wrote:
> > >On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> > >>To support kick in secondary process, we propose callfd_pri and
> > >>kickfd_pri to store the value in primary process; and by a new
> > >>API, rte_vhost_set_vring_effective_fd(), we can set effective
> > >>callfd and kickfd which can be used by secondary process.
> > >>
> > >>Note in this case, either primary process or the secondary process
> > >>can kick the frontend; that is, they cannot kick a vring at the
> > >>same time.
> > >Since only one can work, why not just overwriting the fd? Say, you
> > >could introudce some APIs like "rte_vhost_set_vring_callfd", then
> > >you don't need to introduce few more fields like "callfd_pri".
> >
> > That cannot address the below case:
> > 1. Primary starts;
> > 2. Secondary one starts; (if we overwrite it without storing it in some
> > other fields)
> > 3. Secondary one exits;
> > 4. Secondary two starts. (primary cannot share the fd with this secondary
> > process now, as this fd does not mean anything to the primary process)
>
> I was thinking that those fds will be retrieved by the primary process
> once? So thsoe it got at beginning are still valid?
Yes, the FDs are valid to primary process at step 1. But After overwriting in step 2, those FDs are not valid to primary.
On Fri, Sep 22, 2017 at 02:30:21AM +0000, Tan, Jianfeng wrote:
>
>
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > Sent: Thursday, September 21, 2017 5:18 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; maxime.coquelin@redhat.com; mtetsuyah@gmail.com
> > Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
> >
> > On Thu, Sep 21, 2017 at 03:04:39PM +0800, Tan, Jianfeng wrote:
> > > >On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> > > >>To support kick in secondary process, we propose callfd_pri and
> > > >>kickfd_pri to store the value in primary process; and by a new
> > > >>API, rte_vhost_set_vring_effective_fd(), we can set effective
> > > >>callfd and kickfd which can be used by secondary process.
> > > >>
> > > >>Note in this case, either primary process or the secondary process
> > > >>can kick the frontend; that is, they cannot kick a vring at the
> > > >>same time.
> > > >Since only one can work, why not just overwriting the fd? Say, you
> > > >could introudce some APIs like "rte_vhost_set_vring_callfd", then
> > > >you don't need to introduce few more fields like "callfd_pri".
> > >
> > > That cannot address the below case:
> > > 1. Primary starts;
> > > 2. Secondary one starts; (if we overwrite it without storing it in some
> > > other fields)
> > > 3. Secondary one exits;
> > > 4. Secondary two starts. (primary cannot share the fd with this secondary
> > > process now, as this fd does not mean anything to the primary process)
> >
> > I was thinking that those fds will be retrieved by the primary process
> > once? So thsoe it got at beginning are still valid?
>
> Yes, the FDs are valid to primary process at step 1. But After overwriting in step 2, those FDs are not valid to primary.
Yes, but the primary process has already got its correct fds saved, right?
With the saved fds, it then could share it with another secondary process.
Actually, the key (and typical) issue of multi-process here is the fds are
process specific, while they are stored in the shared memory. That means
only one will take effect eventually. Worse, the old ones are lost.
So, I think to make it right in this case, you should move the fds from
the shared memory and store them in the memory of the corresponding process.
If that's done, all processes could have its own valid fds, then every
process could do the kick (if that's really necessary).
You could check following commit for more info.
553f45932fb7 ("net/virtio: store PCI operators pointer locally")
--yliu
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Wednesday, September 27, 2017 5:36 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; mtetsuyah@gmail.com
> Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
>
> On Fri, Sep 22, 2017 at 02:30:21AM +0000, Tan, Jianfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > > Sent: Thursday, September 21, 2017 5:18 PM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> mtetsuyah@gmail.com
> > > Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
> > >
> > > On Thu, Sep 21, 2017 at 03:04:39PM +0800, Tan, Jianfeng wrote:
> > > > >On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> > > > >>To support kick in secondary process, we propose callfd_pri and
> > > > >>kickfd_pri to store the value in primary process; and by a new
> > > > >>API, rte_vhost_set_vring_effective_fd(), we can set effective
> > > > >>callfd and kickfd which can be used by secondary process.
> > > > >>
> > > > >>Note in this case, either primary process or the secondary process
> > > > >>can kick the frontend; that is, they cannot kick a vring at the
> > > > >>same time.
> > > > >Since only one can work, why not just overwriting the fd? Say, you
> > > > >could introudce some APIs like "rte_vhost_set_vring_callfd", then
> > > > >you don't need to introduce few more fields like "callfd_pri".
> > > >
> > > > That cannot address the below case:
> > > > 1. Primary starts;
> > > > 2. Secondary one starts; (if we overwrite it without storing it in some
> > > > other fields)
> > > > 3. Secondary one exits;
> > > > 4. Secondary two starts. (primary cannot share the fd with this
> secondary
> > > > process now, as this fd does not mean anything to the primary process)
> > >
> > > I was thinking that those fds will be retrieved by the primary process
> > > once? So thsoe it got at beginning are still valid?
> >
> > Yes, the FDs are valid to primary process at step 1. But After overwriting in
> step 2, those FDs are not valid to primary.
>
> Yes, but the primary process has already got its correct fds saved, right?
> With the saved fds, it then could share it with another secondary process.
>
> Actually, the key (and typical) issue of multi-process here is the fds are
> process specific, while they are stored in the shared memory. That means
> only one will take effect eventually. Worse, the old ones are lost.
>
> So, I think to make it right in this case, you should move the fds from
> the shared memory and store them in the memory of the corresponding
> process.
> If that's done, all processes could have its own valid fds, then every
> process could do the kick (if that's really necessary).
Agreed, that can reduce the application's effort to decide which process should set the FDs. Will try to fix that in the coming version.
Thanks,
Jianfeng
>
> You could check following commit for more info.
> 553f45932fb7 ("net/virtio: store PCI operators pointer locally")
>
> --yliu
> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> Sent: Wednesday, September 27, 2017 5:36 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; mtetsuyah@gmail.com
> Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
>
> On Fri, Sep 22, 2017 at 02:30:21AM +0000, Tan, Jianfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
> > > Sent: Thursday, September 21, 2017 5:18 PM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> mtetsuyah@gmail.com
> > > Subject: Re: [PATCH 10/12] vhost: support to kick in secondary process
> > >
> > > On Thu, Sep 21, 2017 at 03:04:39PM +0800, Tan, Jianfeng wrote:
> > > > >On Fri, Aug 25, 2017 at 09:40:50AM +0000, Jianfeng Tan wrote:
> > > > >>To support kick in secondary process, we propose callfd_pri and
> > > > >>kickfd_pri to store the value in primary process; and by a new
> > > > >>API, rte_vhost_set_vring_effective_fd(), we can set effective
> > > > >>callfd and kickfd which can be used by secondary process.
> > > > >>
> > > > >>Note in this case, either primary process or the secondary process
> > > > >>can kick the frontend; that is, they cannot kick a vring at the
> > > > >>same time.
> > > > >Since only one can work, why not just overwriting the fd? Say, you
> > > > >could introudce some APIs like "rte_vhost_set_vring_callfd", then
> > > > >you don't need to introduce few more fields like "callfd_pri".
> > > >
> > > > That cannot address the below case:
> > > > 1. Primary starts;
> > > > 2. Secondary one starts; (if we overwrite it without storing it in some
> > > > other fields)
> > > > 3. Secondary one exits;
> > > > 4. Secondary two starts. (primary cannot share the fd with this
> secondary
> > > > process now, as this fd does not mean anything to the primary process)
> > >
> > > I was thinking that those fds will be retrieved by the primary process
> > > once? So thsoe it got at beginning are still valid?
> >
> > Yes, the FDs are valid to primary process at step 1. But After overwriting in
> step 2, those FDs are not valid to primary.
>
> Yes, but the primary process has already got its correct fds saved, right?
> With the saved fds, it then could share it with another secondary process.
>
> Actually, the key (and typical) issue of multi-process here is the fds are
> process specific, while they are stored in the shared memory. That means
> only one will take effect eventually. Worse, the old ones are lost.
>
> So, I think to make it right in this case, you should move the fds from
> the shared memory and store them in the memory of the corresponding
> process.
> If that's done, all processes could have its own valid fds, then every
> process could do the kick (if that's really necessary).
>
> You could check following commit for more info.
> 553f45932fb7 ("net/virtio: store PCI operators pointer locally")
Have referred to the above solution, but seems not feasible for this case since there are too many queues. For example, if we define an array like this:
int vhost_callfds[index_by_vid][index_by_queue_id];
The size would be MAX_VHOST_DEVICE * VHOST_MAX_VRING * 8Byte = 2Mbyte.
Instead, can we propose something like process_id to index array located at shared memory?
Thanks,
Jianfeng
On Thu, Sep 28, 2017 at 08:09:38AM +0000, Tan, Jianfeng wrote:
> > Actually, the key (and typical) issue of multi-process here is the fds are
> > process specific, while they are stored in the shared memory. That means
> > only one will take effect eventually. Worse, the old ones are lost.
> >
> > So, I think to make it right in this case, you should move the fds from
> > the shared memory and store them in the memory of the corresponding
> > process.
> > If that's done, all processes could have its own valid fds, then every
> > process could do the kick (if that's really necessary).
> >
> > You could check following commit for more info.
> > 553f45932fb7 ("net/virtio: store PCI operators pointer locally")
>
> Have referred to the above solution, but seems not feasible for this case since there are too many queues. For example, if we define an array like this:
> int vhost_callfds[index_by_vid][index_by_queue_id];
> The size would be MAX_VHOST_DEVICE * VHOST_MAX_VRING * 8Byte = 2Mbyte.
I think you can do it in a dynamic way, like what we did for vhost_dev
allocation?
--yliu
>
> Instead, can we propose something like process_id to index array located at shared memory?
>
> Thanks,
> Jianfeng
On 9/30/2017 4:18 PM, Yuanhan Liu wrote:
> On Thu, Sep 28, 2017 at 08:09:38AM +0000, Tan, Jianfeng wrote:
>>> Actually, the key (and typical) issue of multi-process here is the fds are
>>> process specific, while they are stored in the shared memory. That means
>>> only one will take effect eventually. Worse, the old ones are lost.
>>>
>>> So, I think to make it right in this case, you should move the fds from
>>> the shared memory and store them in the memory of the corresponding
>>> process.
>>> If that's done, all processes could have its own valid fds, then every
>>> process could do the kick (if that's really necessary).
>>>
>>> You could check following commit for more info.
>>> 553f45932fb7 ("net/virtio: store PCI operators pointer locally")
>> Have referred to the above solution, but seems not feasible for this case since there are too many queues. For example, if we define an array like this:
>> int vhost_callfds[index_by_vid][index_by_queue_id];
>> The size would be MAX_VHOST_DEVICE * VHOST_MAX_VRING * 8Byte = 2Mbyte.
> I think you can do it in a dynamic way, like what we did for vhost_dev
> allocation?
I'll give it a try, thanks!
Thanks,
Jianfeng
>
> --yliu
>> Instead, can we propose something like process_id to index array located at shared memory?
>>
>> Thanks,
>> Jianfeng
@@ -432,6 +432,9 @@ int rte_vhost_get_mem_table(int vid, struct rte_vhost_memory **mem);
int rte_vhost_get_vhost_vring(int vid, uint16_t vring_idx,
struct rte_vhost_vring *vring);
+int rte_vhost_set_vring_effective_fd(int vid, uint16_t vring_idx,
+ int callfd, int kickfd);
+
/**
* Get vhost RX queue avail count.
*
@@ -435,13 +435,36 @@ rte_vhost_get_vhost_vring(int vid, uint16_t vring_idx,
vring->used = vq->used;
vring->log_guest_addr = vq->log_guest_addr;
- vring->callfd = vq->callfd;
- vring->kickfd = vq->kickfd;
+ vring->callfd = vq->callfd_pri;
+ vring->kickfd = vq->kickfd_pri;
vring->size = vq->size;
return 0;
}
+int
+rte_vhost_set_vring_effective_fd(int vid, uint16_t vring_idx,
+ int callfd, int kickfd)
+{
+ struct virtio_net *dev;
+ struct vhost_virtqueue *vq;
+
+ dev = get_device(vid);
+ if (!dev)
+ return -1;
+
+ if (vring_idx >= VHOST_MAX_VRING)
+ return -1;
+
+ vq = dev->virtqueue[vring_idx];
+ if (!vq)
+ return -1;
+
+ vq->callfd = callfd;
+ vq->kickfd = kickfd;
+
+ return 0;
+}
uint16_t
rte_vhost_avail_entries(int vid, uint16_t queue_id)
{
@@ -98,9 +98,12 @@ struct vhost_virtqueue {
/* Backend value to determine if device should started/stopped */
int backend;
/* Used to notify the guest (trigger interrupt) */
+ int callfd_pri;
int callfd;
/* Currently unused as polling mode is enabled */
+ int kickfd_pri;
int kickfd;
+
int enabled;
/* Physical address of used ring, for logging */
@@ -664,7 +664,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
if (vq->callfd >= 0)
close(vq->callfd);
- vq->callfd = file.fd;
+ vq->callfd_pri = vq->callfd = file.fd;
}
static void
@@ -684,7 +684,7 @@ vhost_user_set_vring_kick(struct virtio_net *dev, struct VhostUserMsg *pmsg)
vq = dev->virtqueue[file.index];
if (vq->kickfd >= 0)
close(vq->kickfd);
- vq->kickfd = file.fd;
+ vq->kickfd_pri = vq->kickfd = file.fd;
}
static void