[v14,4/6] drivers/net: enable hotplug on secondary process
diff mbox series

Message ID 20180810004213.44497-5-qi.z.zhang@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • enable hotplug on multi-process
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Zhang, Qi Z Aug. 10, 2018, 12:42 a.m. UTC
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

Andrew Rybchenko Aug. 12, 2018, 10:59 a.m. UTC | #1
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.
Zhang, Qi Z Aug. 15, 2018, 1:14 a.m. UTC | #2
> -----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

Patch
diff mbox series

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index eb3cce3a6..80a390b19 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -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);
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 58f7377c6..47d0f23cc 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -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;
diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index 085bb8452..3ce14e2b1 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -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;
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 244f86545..ebe3e35f2 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -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);
diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 0f3d5d673..6d044c862 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -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();
 
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index e8810a171..c601b45f0 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -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);
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index feb92b48e..7794a49bb 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -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;
 
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index e58f32211..992f53b56 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -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]);