* [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. @ 2016-09-13 13:47 Ciara Loftus 2016-09-13 15:10 ` Thomas Monjalon 2016-09-29 9:21 ` Mcnamara, John 0 siblings, 2 replies; 27+ messages in thread From: Ciara Loftus @ 2016-09-13 13:47 UTC (permalink / raw) To: dev In some cases when using the vHost PMD, certain vHost library functions may still need to be accessed. One such example is the rte_vhost_get_queue_num function which returns the number of virtqueues reported by the guest - information which is not exposed by the PMD. This commit introduces a new rte_eth_vhost function that returns the 'vid' associated with a given port id. This allows the PMD user to call vHost library functions which require the 'vid' value. Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- drivers/net/vhost/rte_eth_vhost.c | 29 +++++++++++++++++++++++++++++ drivers/net/vhost/rte_eth_vhost.h | 9 +++++++++ drivers/net/vhost/rte_pmd_vhost_version.map | 6 ++++++ 3 files changed, 44 insertions(+) diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c index 7539cd4..091517d 100644 --- a/drivers/net/vhost/rte_eth_vhost.c +++ b/drivers/net/vhost/rte_eth_vhost.c @@ -428,6 +428,35 @@ rte_eth_vhost_get_queue_event(uint8_t port_id, return -1; } +int +rte_eth_vhost_get_vid_from_port_id(uint8_t port_id) +{ + struct internal_list *list; + struct rte_eth_dev *eth_dev; + struct vhost_queue *vq; + int vid = -1; + + if (!rte_eth_dev_is_valid_port(port_id)) + return -1; + + pthread_mutex_lock(&internal_list_lock); + + TAILQ_FOREACH(list, &internal_list, next) { + eth_dev = list->eth_dev; + if (eth_dev->data->port_id == port_id) { + vq = eth_dev->data->rx_queues[0]; + if (vq) { + vid = vq->vid; + } + break; + } + } + + pthread_mutex_unlock(&internal_list_lock); + + return vid; +} + static void * vhost_driver_session(void *param __rte_unused) { diff --git a/drivers/net/vhost/rte_eth_vhost.h b/drivers/net/vhost/rte_eth_vhost.h index ff5d877..7c98b1a 100644 --- a/drivers/net/vhost/rte_eth_vhost.h +++ b/drivers/net/vhost/rte_eth_vhost.h @@ -102,6 +102,15 @@ struct rte_eth_vhost_queue_event { int rte_eth_vhost_get_queue_event(uint8_t port_id, struct rte_eth_vhost_queue_event *event); +/** + * Get the 'vid' value associated with the specified port. + * + * @return + * - On success, the 'vid' associated with 'port_id'. + * - On failure, a negative value. + */ +int rte_eth_vhost_get_vid_from_port_id(uint8_t port_id); + #ifdef __cplusplus } #endif diff --git a/drivers/net/vhost/rte_pmd_vhost_version.map b/drivers/net/vhost/rte_pmd_vhost_version.map index 65bf3a8..0533a83 100644 --- a/drivers/net/vhost/rte_pmd_vhost_version.map +++ b/drivers/net/vhost/rte_pmd_vhost_version.map @@ -8,3 +8,9 @@ DPDK_16.04 { local: *; }; + +DPDK_16.11 { + global: + + rte_eth_vhost_get_vid_from_port_id; +} -- 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. 2016-09-13 13:47 [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id Ciara Loftus @ 2016-09-13 15:10 ` Thomas Monjalon 2016-09-14 4:43 ` Yuanhan Liu 2016-09-28 16:59 ` Mcnamara, John 2016-09-29 9:21 ` Mcnamara, John 1 sibling, 2 replies; 27+ messages in thread From: Thomas Monjalon @ 2016-09-13 15:10 UTC (permalink / raw) To: Ciara Loftus; +Cc: dev 2016-09-13 14:47, Ciara Loftus: > In some cases when using the vHost PMD, certain vHost library functions > may still need to be accessed. One such example is the > rte_vhost_get_queue_num function which returns the number of virtqueues > reported by the guest - information which is not exposed by the PMD. > > This commit introduces a new rte_eth_vhost function that returns the > 'vid' associated with a given port id. This allows the PMD user to call > vHost library functions which require the 'vid' value. I think we should not add any API to the PMDs. Maybe you are looking for a generic API in ethdev. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-13 15:10 ` Thomas Monjalon @ 2016-09-14 4:43 ` Yuanhan Liu 2016-09-14 7:10 ` Thomas Monjalon 2016-09-28 16:59 ` Mcnamara, John 1 sibling, 1 reply; 27+ messages in thread From: Yuanhan Liu @ 2016-09-14 4:43 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > 2016-09-13 14:47, Ciara Loftus: > > In some cases when using the vHost PMD, certain vHost library functions > > may still need to be accessed. One such example is the > > rte_vhost_get_queue_num function which returns the number of virtqueues > > reported by the guest - information which is not exposed by the PMD. > > > > This commit introduces a new rte_eth_vhost function that returns the > > 'vid' associated with a given port id. This allows the PMD user to call > > vHost library functions which require the 'vid' value. > > I think we should not add any API to the PMDs. In general, I agree with you. > Maybe you are looking for a generic API in ethdev. But maybe it's a bit hard to define a "right" generic API here. For this case, the generic API I can think of could be: - an API to get queue num, like rte_eth_get_queue_enabled_num I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. - an API to get a PMD driver private (or specific) data. For vhost-pmd, it's vid. Again, I don't know what it could be for other nic drivers. This one may be a better option here, because it expose a key field to the application, vid, with which the application can invoke more vhost APIs. And apparently, it's not feasible to try to define a generic API for some (if not each) vhost APIs. Or, better ideas? --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-14 4:43 ` Yuanhan Liu @ 2016-09-14 7:10 ` Thomas Monjalon 2016-09-14 7:21 ` Yuanhan Liu 0 siblings, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-14 7:10 UTC (permalink / raw) To: Yuanhan Liu, Ciara Loftus; +Cc: dev 2016-09-14 12:43, Yuanhan Liu: > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > 2016-09-13 14:47, Ciara Loftus: > > > In some cases when using the vHost PMD, certain vHost library functions > > > may still need to be accessed. One such example is the > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > reported by the guest - information which is not exposed by the PMD. > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > 'vid' associated with a given port id. This allows the PMD user to call > > > vHost library functions which require the 'vid' value. > > > > I think we should not add any API to the PMDs. > > In general, I agree with you. > > > Maybe you are looking for a generic API in ethdev. > > But maybe it's a bit hard to define a "right" generic API here. For this > case, the generic API I can think of could be: > > - an API to get queue num, like rte_eth_get_queue_enabled_num > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > - an API to get a PMD driver private (or specific) data. > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > drivers. > > This one may be a better option here, because it expose a key field to > the application, vid, with which the application can invoke more vhost > APIs. And apparently, it's not feasible to try to define a generic API > for some (if not each) vhost APIs. There could be a similar need in other PMD. If we can get an opaque identifier of the device which is not the port id, we could call some specific functions of the driver not implemented in the generic ethdev API. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-14 7:10 ` Thomas Monjalon @ 2016-09-14 7:21 ` Yuanhan Liu 2016-09-14 8:35 ` Thomas Monjalon 0 siblings, 1 reply; 27+ messages in thread From: Yuanhan Liu @ 2016-09-14 7:21 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > 2016-09-14 12:43, Yuanhan Liu: > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > 2016-09-13 14:47, Ciara Loftus: > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > may still need to be accessed. One such example is the > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > vHost library functions which require the 'vid' value. > > > > > > I think we should not add any API to the PMDs. > > > > In general, I agree with you. > > > > > Maybe you are looking for a generic API in ethdev. > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > case, the generic API I can think of could be: > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > - an API to get a PMD driver private (or specific) data. > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > drivers. > > > > This one may be a better option here, because it expose a key field to > > the application, vid, with which the application can invoke more vhost > > APIs. And apparently, it's not feasible to try to define a generic API > > for some (if not each) vhost APIs. > > There could be a similar need in other PMD. > If we can get an opaque identifier of the device which is not the port id, > we could call some specific functions of the driver not implemented in > the generic ethdev API. That means you have to add/export the PMD API first. Isn't it against what you are proposing -- "I think we should not add any API to the PMDs" ;) --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-14 7:21 ` Yuanhan Liu @ 2016-09-14 8:35 ` Thomas Monjalon 2016-09-18 8:27 ` Yuanhan Liu 0 siblings, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-14 8:35 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Ciara Loftus, dev 2016-09-14 15:21, Yuanhan Liu: > On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > > 2016-09-14 12:43, Yuanhan Liu: > > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > > 2016-09-13 14:47, Ciara Loftus: > > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > > may still need to be accessed. One such example is the > > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > > vHost library functions which require the 'vid' value. > > > > > > > > I think we should not add any API to the PMDs. > > > > > > In general, I agree with you. > > > > > > > Maybe you are looking for a generic API in ethdev. > > > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > > case, the generic API I can think of could be: > > > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > > > - an API to get a PMD driver private (or specific) data. > > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > > drivers. > > > > > > This one may be a better option here, because it expose a key field to > > > the application, vid, with which the application can invoke more vhost > > > APIs. And apparently, it's not feasible to try to define a generic API > > > for some (if not each) vhost APIs. > > > > There could be a similar need in other PMD. > > If we can get an opaque identifier of the device which is not the port id, > > we could call some specific functions of the driver not implemented in > > the generic ethdev API. > > That means you have to add/export the PMD API first. Isn't it against what > you are proposing -- "I think we should not add any API to the PMDs" ;) Yes you are totally right :) Except that in vhost case, we would not have any API in the PMD. But it would allow to have some specific API in other PMDs for the features which do not fit in a generic API. Just a thought, not sure yet. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-14 8:35 ` Thomas Monjalon @ 2016-09-18 8:27 ` Yuanhan Liu 2016-09-21 13:07 ` Thomas Monjalon 0 siblings, 1 reply; 27+ messages in thread From: Yuanhan Liu @ 2016-09-18 8:27 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev On Wed, Sep 14, 2016 at 10:35:53AM +0200, Thomas Monjalon wrote: > 2016-09-14 15:21, Yuanhan Liu: > > On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > > > 2016-09-14 12:43, Yuanhan Liu: > > > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > > > 2016-09-13 14:47, Ciara Loftus: > > > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > > > may still need to be accessed. One such example is the > > > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > > > vHost library functions which require the 'vid' value. > > > > > > > > > > I think we should not add any API to the PMDs. > > > > > > > > In general, I agree with you. > > > > > > > > > Maybe you are looking for a generic API in ethdev. > > > > > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > > > case, the generic API I can think of could be: > > > > > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > > > > > - an API to get a PMD driver private (or specific) data. > > > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > > > drivers. > > > > > > > > This one may be a better option here, because it expose a key field to > > > > the application, vid, with which the application can invoke more vhost > > > > APIs. And apparently, it's not feasible to try to define a generic API > > > > for some (if not each) vhost APIs. > > > > > > There could be a similar need in other PMD. > > > If we can get an opaque identifier of the device which is not the port id, > > > we could call some specific functions of the driver not implemented in > > > the generic ethdev API. > > > > That means you have to add/export the PMD API first. Isn't it against what > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > Yes you are totally right :) > Except that in vhost case, we would not have any API in the PMD. > But it would allow to have some specific API in other PMDs for the features > which do not fit in a generic API. So, does that mean you are okay with this patch now? I mean, okay to introduce a vhost PMD API? --yliu > Just a thought, not sure yet. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-18 8:27 ` Yuanhan Liu @ 2016-09-21 13:07 ` Thomas Monjalon 2016-09-22 2:36 ` Yuanhan Liu 0 siblings, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-21 13:07 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Ciara Loftus, dev 2016-09-18 16:27, Yuanhan Liu: > On Wed, Sep 14, 2016 at 10:35:53AM +0200, Thomas Monjalon wrote: > > 2016-09-14 15:21, Yuanhan Liu: > > > On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > > > > 2016-09-14 12:43, Yuanhan Liu: > > > > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > > > > 2016-09-13 14:47, Ciara Loftus: > > > > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > > > > may still need to be accessed. One such example is the > > > > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > > > > vHost library functions which require the 'vid' value. > > > > > > > > > > > > I think we should not add any API to the PMDs. > > > > > > > > > > In general, I agree with you. > > > > > > > > > > > Maybe you are looking for a generic API in ethdev. > > > > > > > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > > > > case, the generic API I can think of could be: > > > > > > > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > > > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > > > > > > > - an API to get a PMD driver private (or specific) data. > > > > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > > > > drivers. > > > > > > > > > > This one may be a better option here, because it expose a key field to > > > > > the application, vid, with which the application can invoke more vhost > > > > > APIs. And apparently, it's not feasible to try to define a generic API > > > > > for some (if not each) vhost APIs. > > > > > > > > There could be a similar need in other PMD. > > > > If we can get an opaque identifier of the device which is not the port id, > > > > we could call some specific functions of the driver not implemented in > > > > the generic ethdev API. > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > Yes you are totally right :) > > Except that in vhost case, we would not have any API in the PMD. > > But it would allow to have some specific API in other PMDs for the features > > which do not fit in a generic API. > > So, does that mean you are okay with this patch now? I mean, okay to introduce > a vhost PMD API? It means I would be in favor of introducing API in drivers for very specific features. In this case, I am not sure that retrieving an internal id is very specific. Opinions are welcome ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-21 13:07 ` Thomas Monjalon @ 2016-09-22 2:36 ` Yuanhan Liu 2016-09-22 16:43 ` Thomas Monjalon 0 siblings, 1 reply; 27+ messages in thread From: Yuanhan Liu @ 2016-09-22 2:36 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev On Wed, Sep 21, 2016 at 03:07:51PM +0200, Thomas Monjalon wrote: > 2016-09-18 16:27, Yuanhan Liu: > > On Wed, Sep 14, 2016 at 10:35:53AM +0200, Thomas Monjalon wrote: > > > 2016-09-14 15:21, Yuanhan Liu: > > > > On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > > > > > 2016-09-14 12:43, Yuanhan Liu: > > > > > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > > > > > 2016-09-13 14:47, Ciara Loftus: > > > > > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > > > > > may still need to be accessed. One such example is the > > > > > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > > > > > vHost library functions which require the 'vid' value. > > > > > > > > > > > > > > I think we should not add any API to the PMDs. > > > > > > > > > > > > In general, I agree with you. > > > > > > > > > > > > > Maybe you are looking for a generic API in ethdev. > > > > > > > > > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > > > > > case, the generic API I can think of could be: > > > > > > > > > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > > > > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > > > > > > > > > - an API to get a PMD driver private (or specific) data. > > > > > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > > > > > drivers. > > > > > > > > > > > > This one may be a better option here, because it expose a key field to > > > > > > the application, vid, with which the application can invoke more vhost > > > > > > APIs. And apparently, it's not feasible to try to define a generic API > > > > > > for some (if not each) vhost APIs. > > > > > > > > > > There could be a similar need in other PMD. > > > > > If we can get an opaque identifier of the device which is not the port id, > > > > > we could call some specific functions of the driver not implemented in > > > > > the generic ethdev API. > > > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > > > Yes you are totally right :) > > > Except that in vhost case, we would not have any API in the PMD. > > > But it would allow to have some specific API in other PMDs for the features > > > which do not fit in a generic API. > > > > So, does that mean you are okay with this patch now? I mean, okay to introduce > > a vhost PMD API? > > It means I would be in favor of introducing API in drivers for very specific > features. > In this case, I am not sure that retrieving an internal id is very specific. It's not, instead, it's very generic. The "internal id" is actually the public interface to vhost-user application, like "fd" to file APIs. Instead of introducing a few specific wrappers/APIs, I'd prefer to introduce a generic one to get the handle, and let the application to call other vhost APIs. --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-22 2:36 ` Yuanhan Liu @ 2016-09-22 16:43 ` Thomas Monjalon 2016-09-23 4:26 ` Yuanhan Liu 0 siblings, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-22 16:43 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Ciara Loftus, dev 2016-09-22 10:36, Yuanhan Liu: > On Wed, Sep 21, 2016 at 03:07:51PM +0200, Thomas Monjalon wrote: > > 2016-09-18 16:27, Yuanhan Liu: > > > On Wed, Sep 14, 2016 at 10:35:53AM +0200, Thomas Monjalon wrote: > > > > 2016-09-14 15:21, Yuanhan Liu: > > > > > On Wed, Sep 14, 2016 at 09:10:48AM +0200, Thomas Monjalon wrote: > > > > > > 2016-09-14 12:43, Yuanhan Liu: > > > > > > > On Tue, Sep 13, 2016 at 05:10:09PM +0200, Thomas Monjalon wrote: > > > > > > > > 2016-09-13 14:47, Ciara Loftus: > > > > > > > > > In some cases when using the vHost PMD, certain vHost library functions > > > > > > > > > may still need to be accessed. One such example is the > > > > > > > > > rte_vhost_get_queue_num function which returns the number of virtqueues > > > > > > > > > reported by the guest - information which is not exposed by the PMD. > > > > > > > > > > > > > > > > > > This commit introduces a new rte_eth_vhost function that returns the > > > > > > > > > 'vid' associated with a given port id. This allows the PMD user to call > > > > > > > > > vHost library functions which require the 'vid' value. > > > > > > > > > > > > > > > > I think we should not add any API to the PMDs. > > > > > > > > > > > > > > In general, I agree with you. > > > > > > > > > > > > > > > Maybe you are looking for a generic API in ethdev. > > > > > > > > > > > > > > But maybe it's a bit hard to define a "right" generic API here. For this > > > > > > > case, the generic API I can think of could be: > > > > > > > > > > > > > > - an API to get queue num, like rte_eth_get_queue_enabled_num > > > > > > > I barely know NIC pmd drivers, but I doubt it's useful/meaningful for them. > > > > > > > > > > > > > > - an API to get a PMD driver private (or specific) data. > > > > > > > For vhost-pmd, it's vid. Again, I don't know what it could be for other nic > > > > > > > drivers. > > > > > > > > > > > > > > This one may be a better option here, because it expose a key field to > > > > > > > the application, vid, with which the application can invoke more vhost > > > > > > > APIs. And apparently, it's not feasible to try to define a generic API > > > > > > > for some (if not each) vhost APIs. > > > > > > > > > > > > There could be a similar need in other PMD. > > > > > > If we can get an opaque identifier of the device which is not the port id, > > > > > > we could call some specific functions of the driver not implemented in > > > > > > the generic ethdev API. > > > > > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > > > > > Yes you are totally right :) > > > > Except that in vhost case, we would not have any API in the PMD. > > > > But it would allow to have some specific API in other PMDs for the features > > > > which do not fit in a generic API. > > > > > > So, does that mean you are okay with this patch now? I mean, okay to introduce > > > a vhost PMD API? > > > > It means I would be in favor of introducing API in drivers for very specific > > features. > > In this case, I am not sure that retrieving an internal id is very specific. > > It's not, instead, it's very generic. The "internal id" is actually the > public interface to vhost-user application, like "fd" to file APIs. > > Instead of introducing a few specific wrappers/APIs, I'd prefer to > introduce a generic one to get the handle, and let the application to > call other vhost APIs. Yes it makes sense. I was thinking of introducing a function to get an internal id from ethdev, in order to use it with any driver or underlying library. But it would be an opaque pointer and you need an int. Note that we can cast an int into a pointer, so I am not sure what is best. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-22 16:43 ` Thomas Monjalon @ 2016-09-23 4:26 ` Yuanhan Liu 2016-09-23 8:43 ` Thomas Monjalon 2016-09-23 21:23 ` Wiles, Keith 0 siblings, 2 replies; 27+ messages in thread From: Yuanhan Liu @ 2016-09-23 4:26 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev, Wiles, Keith On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > > > There could be a similar need in other PMD. > > > > > > > If we can get an opaque identifier of the device which is not the port id, > > > > > > > we could call some specific functions of the driver not implemented in > > > > > > > the generic ethdev API. > > > > > > > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > > > > > > > Yes you are totally right :) > > > > > Except that in vhost case, we would not have any API in the PMD. > > > > > But it would allow to have some specific API in other PMDs for the features > > > > > which do not fit in a generic API. > > > > > > > > So, does that mean you are okay with this patch now? I mean, okay to introduce > > > > a vhost PMD API? > > > > > > It means I would be in favor of introducing API in drivers for very specific > > > features. > > > In this case, I am not sure that retrieving an internal id is very specific. > > > > It's not, instead, it's very generic. The "internal id" is actually the > > public interface to vhost-user application, like "fd" to file APIs. > > > > Instead of introducing a few specific wrappers/APIs, I'd prefer to > > introduce a generic one to get the handle, and let the application to > > call other vhost APIs. > > Yes it makes sense. > I was thinking of introducing a function to get an internal id from ethdev, > in order to use it with any driver or underlying library. > But it would be an opaque pointer and you need an int. > Note that we can cast an int into a pointer, so I am not sure what is best. Yes, that should work. But I just doubt what the "opaque pointer" could be for other PMD drivers, and what the application could do with it. For a typical nic PMD driver, I can think of nothing is valuable to export to user applications. But maybe it's valuable to other virtual PMD drives as well, like the TAP pmd from Keith? If so, we may go that way. Another thought is that, it may be a bit weird to me to introduce an API to get an opaque pointer. I mean, it's a bit hard to document it, because it has different meaning for different drivers. Should we list all of them then? --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 4:26 ` Yuanhan Liu @ 2016-09-23 8:43 ` Thomas Monjalon 2016-09-23 9:16 ` Yuanhan Liu 2016-09-23 21:23 ` Wiles, Keith 1 sibling, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-23 8:43 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Ciara Loftus, dev, Wiles, Keith 2016-09-23 12:26, Yuanhan Liu: > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > > > > There could be a similar need in other PMD. > > > > > > > > If we can get an opaque identifier of the device which is not the port id, > > > > > > > > we could call some specific functions of the driver not implemented in > > > > > > > > the generic ethdev API. > > > > > > > > > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > > > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > > > > > > > > > Yes you are totally right :) > > > > > > Except that in vhost case, we would not have any API in the PMD. > > > > > > But it would allow to have some specific API in other PMDs for the features > > > > > > which do not fit in a generic API. > > > > > > > > > > So, does that mean you are okay with this patch now? I mean, okay to introduce > > > > > a vhost PMD API? > > > > > > > > It means I would be in favor of introducing API in drivers for very specific > > > > features. > > > > In this case, I am not sure that retrieving an internal id is very specific. > > > > > > It's not, instead, it's very generic. The "internal id" is actually the > > > public interface to vhost-user application, like "fd" to file APIs. > > > > > > Instead of introducing a few specific wrappers/APIs, I'd prefer to > > > introduce a generic one to get the handle, and let the application to > > > call other vhost APIs. > > > > Yes it makes sense. > > I was thinking of introducing a function to get an internal id from ethdev, > > in order to use it with any driver or underlying library. > > But it would be an opaque pointer and you need an int. > > Note that we can cast an int into a pointer, so I am not sure what is best. > > Yes, that should work. But I just doubt what the "opaque pointer" could be > for other PMD drivers, and what the application could do with it. For a > typical nic PMD driver, I can think of nothing is valuable to export to > user applications. > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > pmd from Keith? > > If so, we may go that way. I would like to have more opinions/votes before proceeding. > Another thought is that, it may be a bit weird to me to introduce an API > to get an opaque pointer. I mean, it's a bit hard to document it, because > it has different meaning for different drivers. Should we list all of > them then? I think it can be documented in API using this handler how it can be retrieved. In your case, the vhost lib can explain that the vid is retrieved from the PMD with this generic ethdev function. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 8:43 ` Thomas Monjalon @ 2016-09-23 9:16 ` Yuanhan Liu 2016-09-23 9:26 ` Loftus, Ciara 0 siblings, 1 reply; 27+ messages in thread From: Yuanhan Liu @ 2016-09-23 9:16 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Ciara Loftus, dev, Wiles, Keith On Fri, Sep 23, 2016 at 10:43:20AM +0200, Thomas Monjalon wrote: > 2016-09-23 12:26, Yuanhan Liu: > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > > > > > There could be a similar need in other PMD. > > > > > > > > > If we can get an opaque identifier of the device which is not the port id, > > > > > > > > > we could call some specific functions of the driver not implemented in > > > > > > > > > the generic ethdev API. > > > > > > > > > > > > > > > > That means you have to add/export the PMD API first. Isn't it against what > > > > > > > > you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > > > > > > > > > > > Yes you are totally right :) > > > > > > > Except that in vhost case, we would not have any API in the PMD. > > > > > > > But it would allow to have some specific API in other PMDs for the features > > > > > > > which do not fit in a generic API. > > > > > > > > > > > > So, does that mean you are okay with this patch now? I mean, okay to introduce > > > > > > a vhost PMD API? > > > > > > > > > > It means I would be in favor of introducing API in drivers for very specific > > > > > features. > > > > > In this case, I am not sure that retrieving an internal id is very specific. > > > > > > > > It's not, instead, it's very generic. The "internal id" is actually the > > > > public interface to vhost-user application, like "fd" to file APIs. > > > > > > > > Instead of introducing a few specific wrappers/APIs, I'd prefer to > > > > introduce a generic one to get the handle, and let the application to > > > > call other vhost APIs. > > > > > > Yes it makes sense. > > > I was thinking of introducing a function to get an internal id from ethdev, > > > in order to use it with any driver or underlying library. > > > But it would be an opaque pointer and you need an int. > > > Note that we can cast an int into a pointer, so I am not sure what is best. > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > for other PMD drivers, and what the application could do with it. For a > > typical nic PMD driver, I can think of nothing is valuable to export to > > user applications. > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > pmd from Keith? > > > > If so, we may go that way. > > I would like to have more opinions/votes before proceeding. Sure, fair enough. There is no rush. > > Another thought is that, it may be a bit weird to me to introduce an API > > to get an opaque pointer. I mean, it's a bit hard to document it, because > > it has different meaning for different drivers. Should we list all of > > them then? > > I think it can be documented in API using this handler how it can > be retrieved. In your case, the vhost lib can explain that the vid > is retrieved from the PMD with this generic ethdev function. Okay. --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 9:16 ` Yuanhan Liu @ 2016-09-23 9:26 ` Loftus, Ciara 0 siblings, 0 replies; 27+ messages in thread From: Loftus, Ciara @ 2016-09-23 9:26 UTC (permalink / raw) To: Yuanhan Liu, Thomas Monjalon; +Cc: dev, Wiles, Keith > On Fri, Sep 23, 2016 at 10:43:20AM +0200, Thomas Monjalon wrote: > > 2016-09-23 12:26, Yuanhan Liu: > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > > > > > > There could be a similar need in other PMD. > > > > > > > > > > If we can get an opaque identifier of the device which is not > the port id, > > > > > > > > > > we could call some specific functions of the driver not > implemented in > > > > > > > > > > the generic ethdev API. > > > > > > > > > > > > > > > > > > That means you have to add/export the PMD API first. Isn't it > against what > > > > > > > > > you are proposing -- "I think we should not add any API to the > PMDs" ;) > > > > > > > > > > > > > > > > Yes you are totally right :) > > > > > > > > Except that in vhost case, we would not have any API in the > PMD. > > > > > > > > But it would allow to have some specific API in other PMDs for > the features > > > > > > > > which do not fit in a generic API. > > > > > > > > > > > > > > So, does that mean you are okay with this patch now? I mean, > okay to introduce > > > > > > > a vhost PMD API? > > > > > > > > > > > > It means I would be in favor of introducing API in drivers for very > specific > > > > > > features. > > > > > > In this case, I am not sure that retrieving an internal id is very > specific. > > > > > > > > > > It's not, instead, it's very generic. The "internal id" is actually the > > > > > public interface to vhost-user application, like "fd" to file APIs. > > > > > > > > > > Instead of introducing a few specific wrappers/APIs, I'd prefer to > > > > > introduce a generic one to get the handle, and let the application to > > > > > call other vhost APIs. > > > > > > > > Yes it makes sense. > > > > I was thinking of introducing a function to get an internal id from > ethdev, > > > > in order to use it with any driver or underlying library. > > > > But it would be an opaque pointer and you need an int. > > > > Note that we can cast an int into a pointer, so I am not sure what is > best. > > > > > > Yes, that should work. But I just doubt what the "opaque pointer" could > be > > > for other PMD drivers, and what the application could do with it. For a > > > typical nic PMD driver, I can think of nothing is valuable to export to > > > user applications. > > > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > > pmd from Keith? > > > > > > If so, we may go that way. > > > > I would like to have more opinions/votes before proceeding. > > Sure, fair enough. There is no rush. My hope would be have this, or at least some way to access rte_vhost_get_queue_num(vid) from the PMD in 16.11. We can't integrate the PMD into OVS until we achieve this. Is this likely at this stage given the uncertainty around the API? Thanks, Ciara > > > > Another thought is that, it may be a bit weird to me to introduce an API > > > to get an opaque pointer. I mean, it's a bit hard to document it, because > > > it has different meaning for different drivers. Should we list all of > > > them then? > > > > I think it can be documented in API using this handler how it can > > be retrieved. In your case, the vhost lib can explain that the vid > > is retrieved from the PMD with this generic ethdev function. > > Okay. > > --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 4:26 ` Yuanhan Liu 2016-09-23 8:43 ` Thomas Monjalon @ 2016-09-23 21:23 ` Wiles, Keith 2016-09-26 3:19 ` Yuanhan Liu 2016-09-26 13:12 ` Thomas Monjalon 1 sibling, 2 replies; 27+ messages in thread From: Wiles, Keith @ 2016-09-23 21:23 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Thomas Monjalon, Loftus, Ciara, dev Regards, Keith > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: >>>>>>>> There could be a similar need in other PMD. >>>>>>>> If we can get an opaque identifier of the device which is not the port id, >>>>>>>> we could call some specific functions of the driver not implemented in >>>>>>>> the generic ethdev API. >>>>>>> >>>>>>> That means you have to add/export the PMD API first. Isn't it against what >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) >>>>>> >>>>>> Yes you are totally right :) >>>>>> Except that in vhost case, we would not have any API in the PMD. >>>>>> But it would allow to have some specific API in other PMDs for the features >>>>>> which do not fit in a generic API. >>>>> >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce >>>>> a vhost PMD API? >>>> >>>> It means I would be in favor of introducing API in drivers for very specific >>>> features. >>>> In this case, I am not sure that retrieving an internal id is very specific. >>> >>> It's not, instead, it's very generic. The "internal id" is actually the >>> public interface to vhost-user application, like "fd" to file APIs. >>> >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to >>> introduce a generic one to get the handle, and let the application to >>> call other vhost APIs. >> >> Yes it makes sense. >> I was thinking of introducing a function to get an internal id from ethdev, >> in order to use it with any driver or underlying library. >> But it would be an opaque pointer and you need an int. >> Note that we can cast an int into a pointer, so I am not sure what is best. > > Yes, that should work. But I just doubt what the "opaque pointer" could be > for other PMD drivers, and what the application could do with it. For a > typical nic PMD driver, I can think of nothing is valuable to export to > user applications. > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > pmd from Keith? I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. The API could be something like this: struct rte_tlv { /* Type/Length/Value like structure */ uint16_t type; /* Type of command */ uint16_t len; /* Length of data section on input and on output */ uint16_t tlen; /* Total or max length of data buffer */ uint8_t data[0]; }; int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > If so, we may go that way. > > Another thought is that, it may be a bit weird to me to introduce an API > to get an opaque pointer. I mean, it's a bit hard to document it, because > it has different meaning for different drivers. Should we list all of > them then? > > --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 21:23 ` Wiles, Keith @ 2016-09-26 3:19 ` Yuanhan Liu 2016-09-26 13:12 ` Thomas Monjalon 1 sibling, 0 replies; 27+ messages in thread From: Yuanhan Liu @ 2016-09-26 3:19 UTC (permalink / raw) To: Wiles, Keith; +Cc: Thomas Monjalon, Loftus, Ciara, dev On Fri, Sep 23, 2016 at 09:23:11PM +0000, Wiles, Keith wrote: > > Regards, > Keith > > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > >>>>>>>> There could be a similar need in other PMD. > >>>>>>>> If we can get an opaque identifier of the device which is not the port id, > >>>>>>>> we could call some specific functions of the driver not implemented in > >>>>>>>> the generic ethdev API. > >>>>>>> > >>>>>>> That means you have to add/export the PMD API first. Isn't it against what > >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) > >>>>>> > >>>>>> Yes you are totally right :) > >>>>>> Except that in vhost case, we would not have any API in the PMD. > >>>>>> But it would allow to have some specific API in other PMDs for the features > >>>>>> which do not fit in a generic API. > >>>>> > >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce > >>>>> a vhost PMD API? > >>>> > >>>> It means I would be in favor of introducing API in drivers for very specific > >>>> features. > >>>> In this case, I am not sure that retrieving an internal id is very specific. > >>> > >>> It's not, instead, it's very generic. The "internal id" is actually the > >>> public interface to vhost-user application, like "fd" to file APIs. > >>> > >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to > >>> introduce a generic one to get the handle, and let the application to > >>> call other vhost APIs. > >> > >> Yes it makes sense. > >> I was thinking of introducing a function to get an internal id from ethdev, > >> in order to use it with any driver or underlying library. > >> But it would be an opaque pointer and you need an int. > >> Note that we can cast an int into a pointer, so I am not sure what is best. > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > for other PMD drivers, and what the application could do with it. For a > > typical nic PMD driver, I can think of nothing is valuable to export to > > user applications. > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > pmd from Keith? > > I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Yes, that's what I meant. > Not sure what any application would need with the FD here, as it could cause some problems. Me, neither. > This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? > > Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. > > The API could be something like this: > struct rte_tlv { /* Type/Length/Value like structure */ > uint16_t type; /* Type of command */ > uint16_t len; /* Length of data section on input and on output */ > uint16_t tlen; /* Total or max length of data buffer */ > uint8_t data[0]; > }; > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); Looks like a very clean solution to me! Thomas? --yliu > > > > > If so, we may go that way. > > > > Another thought is that, it may be a bit weird to me to introduce an API > > to get an opaque pointer. I mean, it's a bit hard to document it, because > > it has different meaning for different drivers. Should we list all of > > them then? > > > > --yliu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-23 21:23 ` Wiles, Keith 2016-09-26 3:19 ` Yuanhan Liu @ 2016-09-26 13:12 ` Thomas Monjalon 2016-09-26 13:18 ` Bruce Richardson 1 sibling, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-26 13:12 UTC (permalink / raw) To: Wiles, Keith; +Cc: Yuanhan Liu, Loftus, Ciara, dev 2016-09-23 21:23, Wiles, Keith: > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > >>>>>>>> There could be a similar need in other PMD. > >>>>>>>> If we can get an opaque identifier of the device which is not the port id, > >>>>>>>> we could call some specific functions of the driver not implemented in > >>>>>>>> the generic ethdev API. > >>>>>>> > >>>>>>> That means you have to add/export the PMD API first. Isn't it against what > >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) > >>>>>> > >>>>>> Yes you are totally right :) > >>>>>> Except that in vhost case, we would not have any API in the PMD. > >>>>>> But it would allow to have some specific API in other PMDs for the features > >>>>>> which do not fit in a generic API. > >>>>> > >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce > >>>>> a vhost PMD API? > >>>> > >>>> It means I would be in favor of introducing API in drivers for very specific > >>>> features. > >>>> In this case, I am not sure that retrieving an internal id is very specific. > >>> > >>> It's not, instead, it's very generic. The "internal id" is actually the > >>> public interface to vhost-user application, like "fd" to file APIs. > >>> > >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to > >>> introduce a generic one to get the handle, and let the application to > >>> call other vhost APIs. > >> > >> Yes it makes sense. > >> I was thinking of introducing a function to get an internal id from ethdev, > >> in order to use it with any driver or underlying library. > >> But it would be an opaque pointer and you need an int. > >> Note that we can cast an int into a pointer, so I am not sure what is best. > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > for other PMD drivers, and what the application could do with it. For a > > typical nic PMD driver, I can think of nothing is valuable to export to > > user applications. > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > pmd from Keith? > > I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. > > This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? > > Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. > > The API could be something like this: > struct rte_tlv { /* Type/Length/Value like structure */ > uint16_t type; /* Type of command */ > uint16_t len; /* Length of data section on input and on output */ > uint16_t tlen; /* Total or max length of data buffer */ > uint8_t data[0]; > }; > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); Yes we are talking about having some specific functions per driver which are not defined in the generic ethdev layer. We need only one function in ethdev to give access to driver-specific API. My idea is to convert the port id into an opaque handler. Your idea is to use the port id in an ioctl like function. About the implementation, these are the 2 differences between my proposal and yours: - You use the well known port id, whereas I need another handler which is understood by the driver. - You need to build a message string which will be decoded by the driver. I propose to directly offer some specific functions in the drivers which are more convenient to use and easier for code review/debug. No conclusion here. I just want to make sure that we are on the same page, and would like to have feedback from others. Thanks ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 13:12 ` Thomas Monjalon @ 2016-09-26 13:18 ` Bruce Richardson 2016-09-26 14:26 ` Thomas Monjalon 0 siblings, 1 reply; 27+ messages in thread From: Bruce Richardson @ 2016-09-26 13:18 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Wiles, Keith, Yuanhan Liu, Loftus, Ciara, dev On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: > 2016-09-23 21:23, Wiles, Keith: > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > >>>>>>>> There could be a similar need in other PMD. > > >>>>>>>> If we can get an opaque identifier of the device which is not the port id, > > >>>>>>>> we could call some specific functions of the driver not implemented in > > >>>>>>>> the generic ethdev API. > > >>>>>>> > > >>>>>>> That means you have to add/export the PMD API first. Isn't it against what > > >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) > > >>>>>> > > >>>>>> Yes you are totally right :) > > >>>>>> Except that in vhost case, we would not have any API in the PMD. > > >>>>>> But it would allow to have some specific API in other PMDs for the features > > >>>>>> which do not fit in a generic API. > > >>>>> > > >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce > > >>>>> a vhost PMD API? > > >>>> > > >>>> It means I would be in favor of introducing API in drivers for very specific > > >>>> features. > > >>>> In this case, I am not sure that retrieving an internal id is very specific. > > >>> > > >>> It's not, instead, it's very generic. The "internal id" is actually the > > >>> public interface to vhost-user application, like "fd" to file APIs. > > >>> > > >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to > > >>> introduce a generic one to get the handle, and let the application to > > >>> call other vhost APIs. > > >> > > >> Yes it makes sense. > > >> I was thinking of introducing a function to get an internal id from ethdev, > > >> in order to use it with any driver or underlying library. > > >> But it would be an opaque pointer and you need an int. > > >> Note that we can cast an int into a pointer, so I am not sure what is best. > > > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > > for other PMD drivers, and what the application could do with it. For a > > > typical nic PMD driver, I can think of nothing is valuable to export to > > > user applications. > > > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > > pmd from Keith? > > > > I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. > > > > This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? > > > > Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. > > > > The API could be something like this: > > struct rte_tlv { /* Type/Length/Value like structure */ > > uint16_t type; /* Type of command */ > > uint16_t len; /* Length of data section on input and on output */ > > uint16_t tlen; /* Total or max length of data buffer */ > > uint8_t data[0]; > > }; > > > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > Yes we are talking about having some specific functions per driver which > are not defined in the generic ethdev layer. > We need only one function in ethdev to give access to driver-specific API. > My idea is to convert the port id into an opaque handler. > Your idea is to use the port id in an ioctl like function. > > About the implementation, these are the 2 differences between my proposal > and yours: > - You use the well known port id, whereas I need another handler which is > understood by the driver. > - You need to build a message string which will be decoded by the driver. > I propose to directly offer some specific functions in the drivers which > are more convenient to use and easier for code review/debug. > > No conclusion here. I just want to make sure that we are on the same page, > and would like to have feedback from others. Thanks I personally don't like the idea of having a generic IOCTL in ethdev. If you want to have NIC-specific functions provided by a driver, that is fine, but any app using those is going to be limited to working only with that driver. In that case, since the driver in question is known, I don't see any reason to go through the ethdev layer. I think it would be much clearer to have the app instead include the driver's header file and call the driver function directly. The #include at the top of the file makes the dependency very clear, and having a function name instead of IOCTL with magic command numbers allows the action take by the function to be clearer too. Regards, /Bruce ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 13:18 ` Bruce Richardson @ 2016-09-26 14:26 ` Thomas Monjalon 2016-09-26 14:34 ` Bruce Richardson 0 siblings, 1 reply; 27+ messages in thread From: Thomas Monjalon @ 2016-09-26 14:26 UTC (permalink / raw) To: Bruce Richardson; +Cc: Wiles, Keith, Yuanhan Liu, Loftus, Ciara, dev 2016-09-26 14:18, Bruce Richardson: > On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: > > 2016-09-23 21:23, Wiles, Keith: > > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > >>>>>>>> There could be a similar need in other PMD. > > > >>>>>>>> If we can get an opaque identifier of the device which is not the port id, > > > >>>>>>>> we could call some specific functions of the driver not implemented in > > > >>>>>>>> the generic ethdev API. > > > >>>>>>> > > > >>>>>>> That means you have to add/export the PMD API first. Isn't it against what > > > >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) > > > >>>>>> > > > >>>>>> Yes you are totally right :) > > > >>>>>> Except that in vhost case, we would not have any API in the PMD. > > > >>>>>> But it would allow to have some specific API in other PMDs for the features > > > >>>>>> which do not fit in a generic API. > > > >>>>> > > > >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce > > > >>>>> a vhost PMD API? > > > >>>> > > > >>>> It means I would be in favor of introducing API in drivers for very specific > > > >>>> features. > > > >>>> In this case, I am not sure that retrieving an internal id is very specific. > > > >>> > > > >>> It's not, instead, it's very generic. The "internal id" is actually the > > > >>> public interface to vhost-user application, like "fd" to file APIs. > > > >>> > > > >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to > > > >>> introduce a generic one to get the handle, and let the application to > > > >>> call other vhost APIs. > > > >> > > > >> Yes it makes sense. > > > >> I was thinking of introducing a function to get an internal id from ethdev, > > > >> in order to use it with any driver or underlying library. > > > >> But it would be an opaque pointer and you need an int. > > > >> Note that we can cast an int into a pointer, so I am not sure what is best. > > > > > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > > > for other PMD drivers, and what the application could do with it. For a > > > > typical nic PMD driver, I can think of nothing is valuable to export to > > > > user applications. > > > > > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > > > pmd from Keith? > > > > > > I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. > > > > > > This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? > > > > > > Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. > > > > > > The API could be something like this: > > > struct rte_tlv { /* Type/Length/Value like structure */ > > > uint16_t type; /* Type of command */ > > > uint16_t len; /* Length of data section on input and on output */ > > > uint16_t tlen; /* Total or max length of data buffer */ > > > uint8_t data[0]; > > > }; > > > > > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > > > Yes we are talking about having some specific functions per driver which > > are not defined in the generic ethdev layer. > > We need only one function in ethdev to give access to driver-specific API. > > My idea is to convert the port id into an opaque handler. > > Your idea is to use the port id in an ioctl like function. > > > > About the implementation, these are the 2 differences between my proposal > > and yours: > > - You use the well known port id, whereas I need another handler which is > > understood by the driver. > > - You need to build a message string which will be decoded by the driver. > > I propose to directly offer some specific functions in the drivers which > > are more convenient to use and easier for code review/debug. > > > > No conclusion here. I just want to make sure that we are on the same page, > > and would like to have feedback from others. Thanks > > I personally don't like the idea of having a generic IOCTL in ethdev. If you > want to have NIC-specific functions provided by a driver, that is fine, but > any app using those is going to be limited to working only with that driver. > > In that case, since the driver in question is known, I don't see any reason > to go through the ethdev layer. I think it would be much clearer to have the > app instead include the driver's header file and call the driver function > directly. The #include at the top of the file makes the dependency very clear, > and having a function name instead of IOCTL with magic command numbers allows > the action take by the function to be clearer too. So you are against an IOCTL API. Me too. You agree that an application can be NIC-specific and include an header file given by the driver to offer very specific features. Me too. My proposal was to convert the port id to an opaque pointer as handler of these driver APIs. After an offline discussion, we agreed that it is not necessary because drivers manage rte_eth_dev struct and port_id through lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev rte_eth_devices[]; Now let's come back to the vhost discussion. The vhost library uses a vid which could be retrieved from a port id thanks to a new function from a vhost PMD header file. I think there is no more objection against this new specific function? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 14:26 ` Thomas Monjalon @ 2016-09-26 14:34 ` Bruce Richardson 2016-09-26 16:24 ` Iremonger, Bernard 0 siblings, 1 reply; 27+ messages in thread From: Bruce Richardson @ 2016-09-26 14:34 UTC (permalink / raw) To: Thomas Monjalon; +Cc: Wiles, Keith, Yuanhan Liu, Loftus, Ciara, dev On Mon, Sep 26, 2016 at 04:26:27PM +0200, Thomas Monjalon wrote: > 2016-09-26 14:18, Bruce Richardson: > > On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: > > > 2016-09-23 21:23, Wiles, Keith: > > > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > > > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > >>>>>>>> There could be a similar need in other PMD. > > > > >>>>>>>> If we can get an opaque identifier of the device which is not the port id, > > > > >>>>>>>> we could call some specific functions of the driver not implemented in > > > > >>>>>>>> the generic ethdev API. > > > > >>>>>>> > > > > >>>>>>> That means you have to add/export the PMD API first. Isn't it against what > > > > >>>>>>> you are proposing -- "I think we should not add any API to the PMDs" ;) > > > > >>>>>> > > > > >>>>>> Yes you are totally right :) > > > > >>>>>> Except that in vhost case, we would not have any API in the PMD. > > > > >>>>>> But it would allow to have some specific API in other PMDs for the features > > > > >>>>>> which do not fit in a generic API. > > > > >>>>> > > > > >>>>> So, does that mean you are okay with this patch now? I mean, okay to introduce > > > > >>>>> a vhost PMD API? > > > > >>>> > > > > >>>> It means I would be in favor of introducing API in drivers for very specific > > > > >>>> features. > > > > >>>> In this case, I am not sure that retrieving an internal id is very specific. > > > > >>> > > > > >>> It's not, instead, it's very generic. The "internal id" is actually the > > > > >>> public interface to vhost-user application, like "fd" to file APIs. > > > > >>> > > > > >>> Instead of introducing a few specific wrappers/APIs, I'd prefer to > > > > >>> introduce a generic one to get the handle, and let the application to > > > > >>> call other vhost APIs. > > > > >> > > > > >> Yes it makes sense. > > > > >> I was thinking of introducing a function to get an internal id from ethdev, > > > > >> in order to use it with any driver or underlying library. > > > > >> But it would be an opaque pointer and you need an int. > > > > >> Note that we can cast an int into a pointer, so I am not sure what is best. > > > > > > > > > > Yes, that should work. But I just doubt what the "opaque pointer" could be > > > > > for other PMD drivers, and what the application could do with it. For a > > > > > typical nic PMD driver, I can think of nothing is valuable to export to > > > > > user applications. > > > > > > > > > > But maybe it's valuable to other virtual PMD drives as well, like the TAP > > > > > pmd from Keith? > > > > > > > > I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. > > > > > > > > This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? > > > > > > > > Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. > > > > > > > > The API could be something like this: > > > > struct rte_tlv { /* Type/Length/Value like structure */ > > > > uint16_t type; /* Type of command */ > > > > uint16_t len; /* Length of data section on input and on output */ > > > > uint16_t tlen; /* Total or max length of data buffer */ > > > > uint8_t data[0]; > > > > }; > > > > > > > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > > > > > Yes we are talking about having some specific functions per driver which > > > are not defined in the generic ethdev layer. > > > We need only one function in ethdev to give access to driver-specific API. > > > My idea is to convert the port id into an opaque handler. > > > Your idea is to use the port id in an ioctl like function. > > > > > > About the implementation, these are the 2 differences between my proposal > > > and yours: > > > - You use the well known port id, whereas I need another handler which is > > > understood by the driver. > > > - You need to build a message string which will be decoded by the driver. > > > I propose to directly offer some specific functions in the drivers which > > > are more convenient to use and easier for code review/debug. > > > > > > No conclusion here. I just want to make sure that we are on the same page, > > > and would like to have feedback from others. Thanks > > > > I personally don't like the idea of having a generic IOCTL in ethdev. If you > > want to have NIC-specific functions provided by a driver, that is fine, but > > any app using those is going to be limited to working only with that driver. > > > > In that case, since the driver in question is known, I don't see any reason > > to go through the ethdev layer. I think it would be much clearer to have the > > app instead include the driver's header file and call the driver function > > directly. The #include at the top of the file makes the dependency very clear, > > and having a function name instead of IOCTL with magic command numbers allows > > the action take by the function to be clearer too. > > So you are against an IOCTL API. Me too. > You agree that an application can be NIC-specific and include an header file > given by the driver to offer very specific features. Me too. > > My proposal was to convert the port id to an opaque pointer as handler of > these driver APIs. After an offline discussion, we agreed that it is not > necessary because drivers manage rte_eth_dev struct and port_id through > lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev rte_eth_devices[]; > +1. I agree with your proposal, and I also agree that no ethdev changes are necessary to support drivers having their own private functions. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 14:34 ` Bruce Richardson @ 2016-09-26 16:24 ` Iremonger, Bernard 2016-09-26 16:55 ` Thomas Monjalon 2016-09-26 17:05 ` Wiles, Keith 0 siblings, 2 replies; 27+ messages in thread From: Iremonger, Bernard @ 2016-09-26 16:24 UTC (permalink / raw) To: Richardson, Bruce, Thomas Monjalon Cc: Wiles, Keith, Yuanhan Liu, Loftus, Ciara, dev Hi Bruce, Thomas, <snip> > Subject: Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' > for a given port id > > On Mon, Sep 26, 2016 at 04:26:27PM +0200, Thomas Monjalon wrote: > > 2016-09-26 14:18, Bruce Richardson: > > > On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: > > > > 2016-09-23 21:23, Wiles, Keith: > > > > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > > > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > >>>>>>>> There could be a similar need in other PMD. > > > > > >>>>>>>> If we can get an opaque identifier of the device which > > > > > >>>>>>>> is not the port id, we could call some specific > > > > > >>>>>>>> functions of the driver not implemented in the generic > ethdev API. > > > > > >>>>>>> > > > > > >>>>>>> That means you have to add/export the PMD API first. > > > > > >>>>>>> Isn't it against what you are proposing -- "I think we > > > > > >>>>>>> should not add any API to the PMDs" ;) > > > > > >>>>>> > > > > > >>>>>> Yes you are totally right :) Except that in vhost case, > > > > > >>>>>> we would not have any API in the PMD. > > > > > >>>>>> But it would allow to have some specific API in other > > > > > >>>>>> PMDs for the features which do not fit in a generic API. > > > > > >>>>> > > > > > >>>>> So, does that mean you are okay with this patch now? I > > > > > >>>>> mean, okay to introduce a vhost PMD API? > > > > > >>>> > > > > > >>>> It means I would be in favor of introducing API in drivers > > > > > >>>> for very specific features. > > > > > >>>> In this case, I am not sure that retrieving an internal id is very > specific. > > > > > >>> > > > > > >>> It's not, instead, it's very generic. The "internal id" is > > > > > >>> actually the public interface to vhost-user application, like "fd" to > file APIs. > > > > > >>> > > > > > >>> Instead of introducing a few specific wrappers/APIs, I'd > > > > > >>> prefer to introduce a generic one to get the handle, and let > > > > > >>> the application to call other vhost APIs. > > > > > >> > > > > > >> Yes it makes sense. > > > > > >> I was thinking of introducing a function to get an internal > > > > > >> id from ethdev, in order to use it with any driver or underlying > library. > > > > > >> But it would be an opaque pointer and you need an int. > > > > > >> Note that we can cast an int into a pointer, so I am not sure what is > best. > > > > > > > > > > > > Yes, that should work. But I just doubt what the "opaque > > > > > > pointer" could be for other PMD drivers, and what the > > > > > > application could do with it. For a typical nic PMD driver, I > > > > > > can think of nothing is valuable to export to user applications. > > > > > > > > > > > > But maybe it's valuable to other virtual PMD drives as well, > > > > > > like the TAP pmd from Keith? > > > > > > > > > > I do not see a need in the TAP PMD other then returning the FD for > TUN/TAP device. Not sure what any application would need with the FD > here, as it could cause some problems. > > > > > > > > > > This feels like we are talking about a IOCTL like generic interface into > the PMD. Then we can add new one types and reject types in the PMD that > are not supported. Would this not be a better method for all future PMD > APIs? > > > > > > > > > > Here is just a thought as to how to solve this problem without a PMD > specific API. A number of current ethdev APIs could be removed to use the > API below. The APIs would be removed from ethdev structure and have the > current APIs use the API below. I know some are not happy with number of > APIs in the ethdev structure. > > > > > > > > > > The API could be something like this: > > > > > struct rte_tlv { /* Type/Length/Value like structure */ > > > > > uint16_t type; /* Type of command */ > > > > > uint16_t len; /* Length of data section on input and on output > */ > > > > > uint16_t tlen; /* Total or max length of data buffer */ > > > > > uint8_t data[0]; > > > > > }; > > > > > > > > > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > > > > > > > Yes we are talking about having some specific functions per driver > > > > which are not defined in the generic ethdev layer. > > > > We need only one function in ethdev to give access to driver-specific > API. > > > > My idea is to convert the port id into an opaque handler. > > > > Your idea is to use the port id in an ioctl like function. > > > > > > > > About the implementation, these are the 2 differences between my > > > > proposal and yours: > > > > - You use the well known port id, whereas I need another handler > > > > which is understood by the driver. > > > > - You need to build a message string which will be decoded by the > driver. > > > > I propose to directly offer some specific functions in the drivers > > > > which are more convenient to use and easier for code review/debug. > > > > > > > > No conclusion here. I just want to make sure that we are on the > > > > same page, and would like to have feedback from others. Thanks > > > > > > I personally don't like the idea of having a generic IOCTL in > > > ethdev. If you want to have NIC-specific functions provided by a > > > driver, that is fine, but any app using those is going to be limited to > working only with that driver. > > > > > > In that case, since the driver in question is known, I don't see any > > > reason to go through the ethdev layer. I think it would be much > > > clearer to have the app instead include the driver's header file and > > > call the driver function directly. The #include at the top of the > > > file makes the dependency very clear, and having a function name > > > instead of IOCTL with magic command numbers allows the action take by > the function to be clearer too. > > > > So you are against an IOCTL API. Me too. > > You agree that an application can be NIC-specific and include an > > header file given by the driver to offer very specific features. Me too. > > > > My proposal was to convert the port id to an opaque pointer as handler > > of these driver APIs. After an offline discussion, we agreed that it > > is not necessary because drivers manage rte_eth_dev struct and port_id > > through > > lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev > > rte_eth_devices[]; > > > > +1. I agree with your proposal, and I also agree that no ethdev changes > +are > necessary to support drivers having their own private functions. I am not sure what has been agreed here. Looking at the code in struct rte_eth_dev{} struct rte_eth_dev{ ... const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ ... } The driver functions are only accessible if they are in struct eth_dev_ops. I thought the issue here was that driver functions should not be added to the struct eth_dev_ops. Hence the need for an eth_dev API to return a pointer to a driver dev_ops structure containing the driver functions. Regards, Bernard. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 16:24 ` Iremonger, Bernard @ 2016-09-26 16:55 ` Thomas Monjalon 2016-09-26 17:05 ` Wiles, Keith 1 sibling, 0 replies; 27+ messages in thread From: Thomas Monjalon @ 2016-09-26 16:55 UTC (permalink / raw) To: Iremonger, Bernard Cc: Richardson, Bruce, Wiles, Keith, Yuanhan Liu, Loftus, Ciara, dev 2016-09-26 16:24, Iremonger, Bernard: > Hi Bruce, Thomas, > > <snip> > > > Subject: Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' > > for a given port id > > > > On Mon, Sep 26, 2016 at 04:26:27PM +0200, Thomas Monjalon wrote: > > > 2016-09-26 14:18, Bruce Richardson: > > > > On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: > > > > > 2016-09-23 21:23, Wiles, Keith: > > > > > > On Sep 23, 2016, at 12:26 AM, Yuanhan Liu > > <yuanhan.liu@linux.intel.com> wrote: > > > > > > > On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: > > > > > > >>>>>>>> There could be a similar need in other PMD. > > > > > > >>>>>>>> If we can get an opaque identifier of the device which > > > > > > >>>>>>>> is not the port id, we could call some specific > > > > > > >>>>>>>> functions of the driver not implemented in the generic > > ethdev API. > > > > > > >>>>>>> > > > > > > >>>>>>> That means you have to add/export the PMD API first. > > > > > > >>>>>>> Isn't it against what you are proposing -- "I think we > > > > > > >>>>>>> should not add any API to the PMDs" ;) > > > > > > >>>>>> > > > > > > >>>>>> Yes you are totally right :) Except that in vhost case, > > > > > > >>>>>> we would not have any API in the PMD. > > > > > > >>>>>> But it would allow to have some specific API in other > > > > > > >>>>>> PMDs for the features which do not fit in a generic API. > > > > > > >>>>> > > > > > > >>>>> So, does that mean you are okay with this patch now? I > > > > > > >>>>> mean, okay to introduce a vhost PMD API? > > > > > > >>>> > > > > > > >>>> It means I would be in favor of introducing API in drivers > > > > > > >>>> for very specific features. > > > > > > >>>> In this case, I am not sure that retrieving an internal id is very > > specific. > > > > > > >>> > > > > > > >>> It's not, instead, it's very generic. The "internal id" is > > > > > > >>> actually the public interface to vhost-user application, like "fd" to > > file APIs. > > > > > > >>> > > > > > > >>> Instead of introducing a few specific wrappers/APIs, I'd > > > > > > >>> prefer to introduce a generic one to get the handle, and let > > > > > > >>> the application to call other vhost APIs. > > > > > > >> > > > > > > >> Yes it makes sense. > > > > > > >> I was thinking of introducing a function to get an internal > > > > > > >> id from ethdev, in order to use it with any driver or underlying > > library. > > > > > > >> But it would be an opaque pointer and you need an int. > > > > > > >> Note that we can cast an int into a pointer, so I am not sure what is > > best. > > > > > > > > > > > > > > Yes, that should work. But I just doubt what the "opaque > > > > > > > pointer" could be for other PMD drivers, and what the > > > > > > > application could do with it. For a typical nic PMD driver, I > > > > > > > can think of nothing is valuable to export to user applications. > > > > > > > > > > > > > > But maybe it's valuable to other virtual PMD drives as well, > > > > > > > like the TAP pmd from Keith? > > > > > > > > > > > > I do not see a need in the TAP PMD other then returning the FD for > > TUN/TAP device. Not sure what any application would need with the FD > > here, as it could cause some problems. > > > > > > > > > > > > This feels like we are talking about a IOCTL like generic interface into > > the PMD. Then we can add new one types and reject types in the PMD that > > are not supported. Would this not be a better method for all future PMD > > APIs? > > > > > > > > > > > > Here is just a thought as to how to solve this problem without a PMD > > specific API. A number of current ethdev APIs could be removed to use the > > API below. The APIs would be removed from ethdev structure and have the > > current APIs use the API below. I know some are not happy with number of > > APIs in the ethdev structure. > > > > > > > > > > > > The API could be something like this: > > > > > > struct rte_tlv { /* Type/Length/Value like structure */ > > > > > > uint16_t type; /* Type of command */ > > > > > > uint16_t len; /* Length of data section on input and on output > > */ > > > > > > uint16_t tlen; /* Total or max length of data buffer */ > > > > > > uint8_t data[0]; > > > > > > }; > > > > > > > > > > > > int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); > > > > > > > > > > Yes we are talking about having some specific functions per driver > > > > > which are not defined in the generic ethdev layer. > > > > > We need only one function in ethdev to give access to driver-specific > > API. > > > > > My idea is to convert the port id into an opaque handler. > > > > > Your idea is to use the port id in an ioctl like function. > > > > > > > > > > About the implementation, these are the 2 differences between my > > > > > proposal and yours: > > > > > - You use the well known port id, whereas I need another handler > > > > > which is understood by the driver. > > > > > - You need to build a message string which will be decoded by the > > driver. > > > > > I propose to directly offer some specific functions in the drivers > > > > > which are more convenient to use and easier for code review/debug. > > > > > > > > > > No conclusion here. I just want to make sure that we are on the > > > > > same page, and would like to have feedback from others. Thanks > > > > > > > > I personally don't like the idea of having a generic IOCTL in > > > > ethdev. If you want to have NIC-specific functions provided by a > > > > driver, that is fine, but any app using those is going to be limited to > > working only with that driver. > > > > > > > > In that case, since the driver in question is known, I don't see any > > > > reason to go through the ethdev layer. I think it would be much > > > > clearer to have the app instead include the driver's header file and > > > > call the driver function directly. The #include at the top of the > > > > file makes the dependency very clear, and having a function name > > > > instead of IOCTL with magic command numbers allows the action take by > > the function to be clearer too. > > > > > > So you are against an IOCTL API. Me too. > > > You agree that an application can be NIC-specific and include an > > > header file given by the driver to offer very specific features. Me too. > > > > > > My proposal was to convert the port id to an opaque pointer as handler > > > of these driver APIs. After an offline discussion, we agreed that it > > > is not necessary because drivers manage rte_eth_dev struct and port_id > > > through > > > lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev > > > rte_eth_devices[]; > > > > > > > +1. I agree with your proposal, and I also agree that no ethdev changes > > +are > > necessary to support drivers having their own private functions. > > I am not sure what has been agreed here. > > Looking at the code in struct rte_eth_dev{} > > struct rte_eth_dev{ > ... > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ > ... > } > > The driver functions are only accessible if they are in struct eth_dev_ops. > I thought the issue here was that driver functions should not be added to the struct eth_dev_ops. > Hence the need for an eth_dev API to return a pointer to a driver dev_ops structure containing the driver functions. An example from an application point of view: #include <rte_pmd_ixgbe.h> rte_pmd_ixgbe_vf_ping(port_id, vf_id); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id 2016-09-26 16:24 ` Iremonger, Bernard 2016-09-26 16:55 ` Thomas Monjalon @ 2016-09-26 17:05 ` Wiles, Keith 1 sibling, 0 replies; 27+ messages in thread From: Wiles, Keith @ 2016-09-26 17:05 UTC (permalink / raw) To: Iremonger, Bernard Cc: Richardson, Bruce, Thomas Monjalon, Yuanhan Liu, Loftus, Ciara, dev Sent from my iPhone On Sep 26, 2016, at 9:24 AM, Iremonger, Bernard <bernard.iremonger@intel.com<mailto:bernard.iremonger@intel.com>> wrote: Hi Bruce, Thomas, <snip> Subject: Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id On Mon, Sep 26, 2016 at 04:26:27PM +0200, Thomas Monjalon wrote: 2016-09-26 14:18, Bruce Richardson: On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote: 2016-09-23 21:23, Wiles, Keith: On Sep 23, 2016, at 12:26 AM, Yuanhan Liu <yuanhan.liu@linux.intel.com<mailto:yuanhan.liu@linux.intel.com>> wrote: On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote: There could be a similar need in other PMD. If we can get an opaque identifier of the device which is not the port id, we could call some specific functions of the driver not implemented in the generic ethdev API. That means you have to add/export the PMD API first. Isn't it against what you are proposing -- "I think we should not add any API to the PMDs" ;) Yes you are totally right :) Except that in vhost case, we would not have any API in the PMD. But it would allow to have some specific API in other PMDs for the features which do not fit in a generic API. So, does that mean you are okay with this patch now? I mean, okay to introduce a vhost PMD API? It means I would be in favor of introducing API in drivers for very specific features. In this case, I am not sure that retrieving an internal id is very specific. It's not, instead, it's very generic. The "internal id" is actually the public interface to vhost-user application, like "fd" to file APIs. Instead of introducing a few specific wrappers/APIs, I'd prefer to introduce a generic one to get the handle, and let the application to call other vhost APIs. Yes it makes sense. I was thinking of introducing a function to get an internal id from ethdev, in order to use it with any driver or underlying library. But it would be an opaque pointer and you need an int. Note that we can cast an int into a pointer, so I am not sure what is best. Yes, that should work. But I just doubt what the "opaque pointer" could be for other PMD drivers, and what the application could do with it. For a typical nic PMD driver, I can think of nothing is valuable to export to user applications. But maybe it's valuable to other virtual PMD drives as well, like the TAP pmd from Keith? I do not see a need in the TAP PMD other then returning the FD for TUN/TAP device. Not sure what any application would need with the FD here, as it could cause some problems. This feels like we are talking about a IOCTL like generic interface into the PMD. Then we can add new one types and reject types in the PMD that are not supported. Would this not be a better method for all future PMD APIs? Here is just a thought as to how to solve this problem without a PMD specific API. A number of current ethdev APIs could be removed to use the API below. The APIs would be removed from ethdev structure and have the current APIs use the API below. I know some are not happy with number of APIs in the ethdev structure. The API could be something like this: struct rte_tlv { /* Type/Length/Value like structure */ uint16_t type; /* Type of command */ uint16_t len; /* Length of data section on input and on output */ uint16_t tlen; /* Total or max length of data buffer */ uint8_t data[0]; }; int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv); Yes we are talking about having some specific functions per driver which are not defined in the generic ethdev layer. We need only one function in ethdev to give access to driver-specific API. My idea is to convert the port id into an opaque handler. Your idea is to use the port id in an ioctl like function. Having a driver specific value is fine by me instead of using port id. But how do we get the specific PMD id value? I think it is a good idea to have something other then port id. About the implementation, these are the 2 differences between my proposal and yours: - You use the well known port id, whereas I need another handler which is understood by the driver. - You need to build a message string which will be decoded by the driver. I propose to directly offer some specific functions in the drivers which are more convenient to use and easier for code review/debug. I think you want a set of apis specific to the driver, but this means everyone will have a different one or could be different. I know Bruce does not like the generic function approach, but to get some type of standard using a generic function like the one I suggested is the only way I see it can work in the long run. The type or command word could be a 32 bit value and the first two bytes use a pmd specific value or one of the common values we define. Say 0x0000 is special set of common one we define and the pmd like TAP could use say 'T' << 16 | 'p' to create the upper 16 bit value that is special to the TAP driver. Not marriaged to the idea just one way to have specific and generic names spaces in the command word. No conclusion here. I just want to make sure that we are on the same page, and would like to have feedback from others. Thanks I personally don't like the idea of having a generic IOCTL in ethdev. If you want to have NIC-specific functions provided by a driver, that is fine, but any app using those is going to be limited to working only with that driver. In that case, since the driver in question is known, I don't see any reason to go through the ethdev layer. I think it would be much clearer to have the app instead include the driver's header file and call the driver function directly. The #include at the top of the file makes the dependency very clear, and having a function name instead of IOCTL with magic command numbers allows the action take by the function to be clearer too. So you are against an IOCTL API. Me too. You agree that an application can be NIC-specific and include an header file given by the driver to offer very specific features. Me too. My proposal was to convert the port id to an opaque pointer as handler of these driver APIs. After an offline discussion, we agreed that it is not necessary because drivers manage rte_eth_dev struct and port_id through lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev rte_eth_devices[]; +1. I agree with your proposal, and I also agree that no ethdev changes +are necessary to support drivers having their own private functions. I am not sure what has been agreed here. Looking at the code in struct rte_eth_dev{} struct rte_eth_dev{ ... const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ ... } The driver functions are only accessible if they are in struct eth_dev_ops. I thought the issue here was that driver functions should not be added to the struct eth_dev_ops. Hence the need for an eth_dev API to return a pointer to a driver dev_ops structure containing the driver functions. Regards, Bernard. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. 2016-09-13 15:10 ` Thomas Monjalon 2016-09-14 4:43 ` Yuanhan Liu @ 2016-09-28 16:59 ` Mcnamara, John 1 sibling, 0 replies; 27+ messages in thread From: Mcnamara, John @ 2016-09-28 16:59 UTC (permalink / raw) To: Thomas Monjalon, Loftus, Ciara; +Cc: dev, Liu, Yuanhan, Richardson, Bruce > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon > Sent: Tuesday, September 13, 2016 4:10 PM > To: Loftus, Ciara <ciara.loftus@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the > 'vid' for a given port id. > > 2016-09-13 14:47, Ciara Loftus: > > In some cases when using the vHost PMD, certain vHost library > > functions may still need to be accessed. One such example is the > > rte_vhost_get_queue_num function which returns the number of > > virtqueues reported by the guest - information which is not exposed by > the PMD. > > > > This commit introduces a new rte_eth_vhost function that returns the > > 'vid' associated with a given port id. This allows the PMD user to > > call vHost library functions which require the 'vid' value. > > I think we should not add any API to the PMDs. > Maybe you are looking for a generic API in ethdev. Hi, This patch got a little lost in a general discussion on generic versus specific APIs. As far as I can see this is a PMD specific API and the general consensus by the end of the thread was that this would be acceptable. Is that the case or are there still objections to this patch. The patch is critical for integration of the Vhost PMD into OvS so I'd like to make sure that we consider it as such. See the discussion here: http://openvswitch.org/pipermail/dev/2016-August/076887.html John ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. 2016-09-13 13:47 [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id Ciara Loftus 2016-09-13 15:10 ` Thomas Monjalon @ 2016-09-29 9:21 ` Mcnamara, John 2016-09-29 9:30 ` Thomas Monjalon 2016-09-29 12:08 ` Yuanhan Liu 1 sibling, 2 replies; 27+ messages in thread From: Mcnamara, John @ 2016-09-29 9:21 UTC (permalink / raw) To: Loftus, Ciara, dev; +Cc: Liu, Yuanhan > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ciara Loftus > Sent: Tuesday, September 13, 2016 2:48 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' > for a given port id. > > In some cases when using the vHost PMD, certain vHost library functions > may still need to be accessed. One such example is the > rte_vhost_get_queue_num function which returns the number of virtqueues > reported by the guest - information which is not exposed by the PMD. > > This commit introduces a new rte_eth_vhost function that returns the 'vid' > associated with a given port id. This allows the PMD user to call vHost > library functions which require the 'vid' value. > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> Acked-by: John McNamara <john.mcnamara@intel.com> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. 2016-09-29 9:21 ` Mcnamara, John @ 2016-09-29 9:30 ` Thomas Monjalon 2016-09-29 12:08 ` Yuanhan Liu 1 sibling, 0 replies; 27+ messages in thread From: Thomas Monjalon @ 2016-09-29 9:30 UTC (permalink / raw) To: Mcnamara, John, Loftus, Ciara, Liu, Yuanhan; +Cc: dev 2016-09-29 09:21, Mcnamara, John: > > In some cases when using the vHost PMD, certain vHost library functions > > may still need to be accessed. One such example is the > > rte_vhost_get_queue_num function which returns the number of virtqueues > > reported by the guest - information which is not exposed by the PMD. > > > > This commit introduces a new rte_eth_vhost function that returns the 'vid' > > associated with a given port id. This allows the PMD user to call vHost > > library functions which require the 'vid' value. > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > Acked-by: John McNamara <john.mcnamara@intel.com> A conclusion to this long thread: This patch can be helpful in many situations where we need vhost lib when using vhost PMD. And driver-specific API is acceptable. And this specific function is not a candidate to be promoted in ethdev. So it should go in release 16.11. When overlooking the OVS issue related, this solution seems to be a workaround. For next release, would it be possible to find a real fix for vhost PMD and/or ethdev API instead of relying on vhost lib? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id. 2016-09-29 9:21 ` Mcnamara, John 2016-09-29 9:30 ` Thomas Monjalon @ 2016-09-29 12:08 ` Yuanhan Liu 1 sibling, 0 replies; 27+ messages in thread From: Yuanhan Liu @ 2016-09-29 12:08 UTC (permalink / raw) To: Mcnamara, John; +Cc: Loftus, Ciara, dev, Liu, Yuanhan On Thu, Sep 29, 2016 at 09:21:11AM +0000, Mcnamara, John wrote: > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ciara Loftus > > Sent: Tuesday, September 13, 2016 2:48 PM > > To: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' > > for a given port id. > > > > In some cases when using the vHost PMD, certain vHost library functions > > may still need to be accessed. One such example is the > > rte_vhost_get_queue_num function which returns the number of virtqueues > > reported by the guest - information which is not exposed by the PMD. > > > > This commit introduces a new rte_eth_vhost function that returns the 'vid' > > associated with a given port id. This allows the PMD user to call vHost > > library functions which require the 'vid' value. > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > Acked-by: John McNamara <john.mcnamara@intel.com> Applied to dpdk-next-virtio, with a fix to fixing missing ";" for the map file (again, caught by my robot). --yliu --- +DPDK_16.11 { + global: + + rte_eth_vhost_get_vid_from_port_id; +} ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-09-29 12:07 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-13 13:47 [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id Ciara Loftus 2016-09-13 15:10 ` Thomas Monjalon 2016-09-14 4:43 ` Yuanhan Liu 2016-09-14 7:10 ` Thomas Monjalon 2016-09-14 7:21 ` Yuanhan Liu 2016-09-14 8:35 ` Thomas Monjalon 2016-09-18 8:27 ` Yuanhan Liu 2016-09-21 13:07 ` Thomas Monjalon 2016-09-22 2:36 ` Yuanhan Liu 2016-09-22 16:43 ` Thomas Monjalon 2016-09-23 4:26 ` Yuanhan Liu 2016-09-23 8:43 ` Thomas Monjalon 2016-09-23 9:16 ` Yuanhan Liu 2016-09-23 9:26 ` Loftus, Ciara 2016-09-23 21:23 ` Wiles, Keith 2016-09-26 3:19 ` Yuanhan Liu 2016-09-26 13:12 ` Thomas Monjalon 2016-09-26 13:18 ` Bruce Richardson 2016-09-26 14:26 ` Thomas Monjalon 2016-09-26 14:34 ` Bruce Richardson 2016-09-26 16:24 ` Iremonger, Bernard 2016-09-26 16:55 ` Thomas Monjalon 2016-09-26 17:05 ` Wiles, Keith 2016-09-28 16:59 ` Mcnamara, John 2016-09-29 9:21 ` Mcnamara, John 2016-09-29 9:30 ` Thomas Monjalon 2016-09-29 12:08 ` Yuanhan Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).