[v14,4/6] drivers/net: enable hotplug on secondary process
Checks
Commit Message
Attach port from secondary should ignore devargs since the private
device is not necessary to support. Also previously, detach port on
a secondary process will mess primary process and cause the same
device can't be attached back again. A secondary process should use
rte_eth_dev_release_port_secondary to release a port.
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
drivers/net/af_packet/rte_eth_af_packet.c | 6 ++++--
drivers/net/bonding/rte_eth_bond_pmd.c | 6 ++++--
drivers/net/kni/rte_eth_kni.c | 6 ++++--
drivers/net/null/rte_eth_null.c | 6 ++++--
drivers/net/octeontx/octeontx_ethdev.c | 8 ++++++++
drivers/net/pcap/rte_eth_pcap.c | 6 ++++--
drivers/net/tap/rte_eth_tap.c | 8 +++++---
drivers/net/vhost/rte_eth_vhost.c | 6 ++++--
8 files changed, 37 insertions(+), 15 deletions(-)
Comments
On 10.08.2018 03:42, Qi Zhang wrote:
> Attach port from secondary should ignore devargs since the private
> device is not necessary to support. Also previously, detach port on
> a secondary process will mess primary process and cause the same
> device can't be attached back again. A secondary process should use
> rte_eth_dev_release_port_secondary to release a port.
>
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
For me, it looks like duplication of the same code logic in
all vdev drivers. I'd say that remove should not be called
at all in the case of secondary process. Also I'd consider
to introduce separate callback for probe in the case of
secondary process: it would make it clear if secondary is
supported and enforce authors to think about secondary
process specifics on probe. As far as I can see it is always
absolutely different branch with own code.
> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Sunday, August 12, 2018 7:00 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net;
> gaetan.rivet@6wind.com; Burakov, Anatoly <anatoly.burakov@intel.com>;
> arybchenko@solarflare.com
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v14 4/6] drivers/net: enable hotplug on
> secondary process
>
> On 10.08.2018 03:42, Qi Zhang wrote:
> > Attach port from secondary should ignore devargs since the private
> > device is not necessary to support. Also previously, detach port on a
> > secondary process will mess primary process and cause the same device
> > can't be attached back again. A secondary process should use
> > rte_eth_dev_release_port_secondary to release a port.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>
> For me, it looks like duplication of the same code logic in all vdev drivers. I'd
> say that remove should not be called at all in the case of secondary process.
But based on current framework, rte_eth_dev_release_port_secondary is required to be called in PMD, and driver->remove is the place to call it as what I see.
> Also I'd consider to introduce separate callback for probe in the case of
> secondary process: it would make it clear if secondary is supported and
> enforce authors to think about secondary process specifics on probe. As far
> as I can see it is always absolutely different branch with own code.
I like this idea. We should give the driver the flexibility to decide to expose device on a secondary process or not.
And this is not for vdev only, maybe another option is adding a flag in rte_driver to indicate if it supports secondary process or not, so we don't need to add callback for all sub bus drivers separately, but in that case we still have to handle secondary in the same probe/remove function if a driver support secondary.
Btw, this looks like involve a lot of change and break ABI. Also, it exceeds the scope of hotplug. I would like to see this in a separate patchset ( better a RFC first), what do you think?
Regards
Qi
@@ -927,8 +927,7 @@ rte_pmd_af_packet_probe(struct rte_vdev_device *dev)
PMD_LOG(INFO, "Initializing pmd_af_packet for %s", name);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(rte_vdev_device_args(dev)) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
PMD_LOG(ERR, "Failed to probe %s", name);
@@ -988,6 +987,9 @@ rte_pmd_af_packet_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
internals = eth_dev->data->dev_private;
for (q = 0; q < internals->nb_queues; q++) {
rte_free(internals->rx_queue[q].rd);
@@ -3186,8 +3186,7 @@ bond_probe(struct rte_vdev_device *dev)
name = rte_vdev_device_name(dev);
RTE_BOND_LOG(INFO, "Initializing pmd_bond for %s", name);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(rte_vdev_device_args(dev)) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
RTE_BOND_LOG(ERR, "Failed to probe %s", name);
@@ -3302,6 +3301,9 @@ bond_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -ENODEV;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
RTE_ASSERT(eth_dev->device == &dev->device);
internals = eth_dev->data->dev_private;
@@ -411,8 +411,7 @@ eth_kni_probe(struct rte_vdev_device *vdev)
params = rte_vdev_device_args(vdev);
PMD_LOG(INFO, "Initializing eth_kni for %s", name);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(params) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
PMD_LOG(ERR, "Failed to probe %s", name);
@@ -465,6 +464,9 @@ eth_kni_remove(struct rte_vdev_device *vdev)
if (eth_dev == NULL)
return -1;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
eth_kni_dev_stop(eth_dev);
internals = eth_dev->data->dev_private;
@@ -615,8 +615,7 @@ rte_pmd_null_probe(struct rte_vdev_device *dev)
params = rte_vdev_device_args(dev);
PMD_LOG(INFO, "Initializing pmd_null for %s", name);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(params) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
PMD_LOG(ERR, "Failed to probe %s", name);
@@ -681,6 +680,9 @@ rte_pmd_null_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
rte_free(eth_dev->data->dev_private);
rte_eth_dev_release_port(eth_dev);
@@ -1141,6 +1141,11 @@ octeontx_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -ENODEV;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ rte_eth_dev_release_port_secondary(eth_dev);
+ continue;
+ }
+
nic = octeontx_pmd_priv(eth_dev);
rte_event_dev_stop(nic->evdev);
PMD_INIT_LOG(INFO, "Closing octeontx device %s", octtx_name);
@@ -1151,6 +1156,9 @@ octeontx_remove(struct rte_vdev_device *dev)
rte_event_dev_close(nic->evdev);
}
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return 0;
+
/* Free FC resource */
octeontx_pko_fc_free();
@@ -1008,8 +1008,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
start_cycles = rte_get_timer_cycles();
hz = rte_get_timer_hz();
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(rte_vdev_device_args(dev)) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
PMD_LOG(ERR, "Failed to probe %s", name);
@@ -1108,6 +1107,9 @@ pmd_pcap_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
rte_free(eth_dev->data->dev_private);
rte_eth_dev_release_port(eth_dev);
@@ -1993,8 +1993,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
name = rte_vdev_device_name(dev);
params = rte_vdev_device_args(dev);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(params) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
TAP_LOG(ERR, "Failed to probe %s", name);
@@ -2076,7 +2075,10 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
/* find the ethdev entry */
eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
if (!eth_dev)
- return 0;
+ return -ENODEV;
+
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
internals = eth_dev->data->dev_private;
@@ -1345,8 +1345,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
VHOST_LOG(INFO, "Initializing pmd_vhost for %s\n", name);
- if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
- strlen(rte_vdev_device_args(dev)) == 0) {
+ if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
eth_dev = rte_eth_dev_attach_secondary(name);
if (!eth_dev) {
VHOST_LOG(ERR, "Failed to probe %s\n", name);
@@ -1437,6 +1436,9 @@ rte_pmd_vhost_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -ENODEV;
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(eth_dev);
+
eth_dev_close(eth_dev);
rte_free(vring_states[eth_dev->data->port_id]);