[dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Wenzhuo Lu]

In testpmd code, device id is used directly to check if bypass
is supported. But APP should not know the details of HW, the NIC
specific info should not be exposed here.
This patch adds a new rte API to check if bypass is supported.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/cmdline.c           | 10 +---------
 drivers/net/ixgbe/ixgbe_bypass.c |  9 +++++++++
 drivers/net/ixgbe/ixgbe_bypass.h |  1 +
 drivers/net/ixgbe/ixgbe_ethdev.c |  1 +
 lib/librte_ether/rte_ethdev.c    | 11 +++++++++++
 lib/librte_ether/rte_ethdev.h    | 16 +++++++++++++++-
 6 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index b6b61ad..eb57329 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -10802,22 +10802,14 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 }
 
 #ifdef RTE_NIC_BYPASS
-#include <rte_pci_dev_ids.h>
 uint8_t
 bypass_is_supported(portid_t port_id)
 {
-	struct rte_port   *port;
-	struct rte_pci_id *pci_id;
-
 	if (port_id_is_invalid(port_id, ENABLED_WARN))
 		return 0;
 
-	/* Get the device id. */
-	port    = &ports[port_id];
-	pci_id = &port->dev_info.pci_dev->id;
-
 	/* Check if NIC supports bypass. */
-	if (pci_id->device_id == IXGBE_DEV_ID_82599_BYPASS) {
+	if (rte_eth_dev_bypass_supported(port_id)) {
 		return 1;
 	}
 	else {
diff --git a/drivers/net/ixgbe/ixgbe_bypass.c b/drivers/net/ixgbe/ixgbe_bypass.c
index 7006928..6142083 100644
--- a/drivers/net/ixgbe/ixgbe_bypass.c
+++ b/drivers/net/ixgbe/ixgbe_bypass.c
@@ -412,3 +412,12 @@ ixgbe_bypass_wd_reset(struct rte_eth_dev *dev)
 
 	return ret_val;
 }
+
+s32
+ixgbe_bypass_supported(struct rte_eth_dev *dev)
+{
+	if (dev->pci_dev->id.device_id == IXGBE_DEV_ID_82599_BYPASS)
+		return 0;
+	else
+		return -ENOTSUP;
+}
diff --git a/drivers/net/ixgbe/ixgbe_bypass.h b/drivers/net/ixgbe/ixgbe_bypass.h
index 5f5c63e..787ae61 100644
--- a/drivers/net/ixgbe/ixgbe_bypass.h
+++ b/drivers/net/ixgbe/ixgbe_bypass.h
@@ -50,6 +50,7 @@ struct ixgbe_bypass_info {
 
 struct rte_eth_dev;
 
+s32 ixgbe_bypass_supported(struct rte_eth_dev *dev);
 void ixgbe_bypass_init(struct rte_eth_dev *dev);
 s32 ixgbe_bypass_state_show(struct rte_eth_dev *dev, u32 *state);
 s32 ixgbe_bypass_state_store(struct rte_eth_dev *dev, u32 *new_state);
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index d478a15..7b7b4ee 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -518,6 +518,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.reta_update          = ixgbe_dev_rss_reta_update,
 	.reta_query           = ixgbe_dev_rss_reta_query,
 #ifdef RTE_NIC_BYPASS
+	.bypass_supported     = ixgbe_bypass_supported,
 	.bypass_init          = ixgbe_bypass_init,
 	.bypass_state_set     = ixgbe_bypass_state_store,
 	.bypass_state_show    = ixgbe_bypass_state_show,
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0a6e3f1..c2e7ff0 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2827,6 +2827,17 @@ rte_eth_dev_rx_intr_disable(uint8_t port_id,
 }
 
 #ifdef RTE_NIC_BYPASS
+int rte_eth_dev_bypass_supported(uint8_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->bypass_supported, -ENOTSUP);
+	return (*dev->dev_ops->bypass_supported)(dev);
+}
+
 int rte_eth_dev_bypass_init(uint8_t port_id)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 4dac364..45a035a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1390,6 +1390,7 @@ enum {
 	((x) == RTE_BYPASS_TMT_OFF || \
 	((x) > RTE_BYPASS_TMT_OFF && (x) < RTE_BYPASS_TMT_NUM))
 
+typedef int32_t (*bypass_supported_t)(struct rte_eth_dev *dev);
 typedef void (*bypass_init_t)(struct rte_eth_dev *dev);
 typedef int32_t (*bypass_state_set_t)(struct rte_eth_dev *dev, uint32_t *new_state);
 typedef int32_t (*bypass_state_show_t)(struct rte_eth_dev *dev, uint32_t *state);
@@ -1494,6 +1495,7 @@ struct eth_dev_ops {
 	/**< Set eeprom */
   /* bypass control */
 #ifdef RTE_NIC_BYPASS
+	bypass_supported_t bypass_supported;
   bypass_init_t bypass_init;
   bypass_state_set_t bypass_state_set;
   bypass_state_show_t bypass_state_show;
@@ -3576,8 +3578,20 @@ int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf,
 			uint16_t tx_rate, uint64_t q_msk);
 
 /**
+ * Check if the bypass functions is supported by a specific device.
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if supported.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-EINVAL) if bad parameter.
+ */
+int rte_eth_dev_bypass_supported(uint8_t port);
+
+/**
  * Initialize bypass logic. This function needs to be called before
- * executing any other bypass API.
+ * executing any other bypass API except rte_eth_dev_bypass_supported.
  *
  * @param port
  *   The port identifier of the Ethernet device.
-- 
1.9.3

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Thomas Monjalon]

2016-07-11 14:21, Wenzhuo Lu:
> In testpmd code, device id is used directly to check if bypass
> is supported. But APP should not know the details of HW, the NIC
> specific info should not be exposed here.

Right

> This patch adds a new rte API to check if bypass is supported.

Hmmm. It's true it is cleaner. But I am not sure having a generic API
for bypass is a good idea at all.
I was thinking to totally remove it.
Maybe we can try to have a specific API by including ixgbe_bypass.h in
the application.

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Wu, Jingjing]

Hi, Wenzhuo

I don't think you need to create a new API: rte_eth_dev_bypass_supported.
If bypass in not supported, the Ops for bypass feature will not be implemented
in driver, or driver need to return NOT supported.

So I think what you need do is just remove the NIC specific things from testpmd,
including "bypass_is_supported".

Thanks
Jingjing

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Monday, July 11, 2016 2:21 PM
> To: dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Lu, Wenzhuo
> Subject: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup
> 
> In testpmd code, device id is used directly to check if bypass is supported.
> But APP should not know the details of HW, the NIC specific info should not
> be exposed here.
> This patch adds a new rte API to check if bypass is supported.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Lu, Wenzhuo]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Monday, July 11, 2016 3:51 PM
> To: Lu, Wenzhuo; dev@dpdk.org
> Cc: thomas.monjalon@6wind.com; Lu, Wenzhuo
> Subject: RE: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup
> 
> Hi, Wenzhuo
> 
> I don't think you need to create a new API: rte_eth_dev_bypass_supported.
> If bypass in not supported, the Ops for bypass feature will not be implemented in
> driver, or driver need to return NOT supported.
> 
> So I think what you need do is just remove the NIC specific things from testpmd,
> including "bypass_is_supported".
Agree, thanks for the comments!

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Lu, Wenzhuo]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, July 11, 2016 3:11 PM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] lib/librte_ether: bypass code cleanup
> 
> 2016-07-11 14:21, Wenzhuo Lu:
> > In testpmd code, device id is used directly to check if bypass is
> > supported. But APP should not know the details of HW, the NIC specific
> > info should not be exposed here.
> 
> Right
> 
> > This patch adds a new rte API to check if bypass is supported.
> 
> Hmmm. It's true it is cleaner. But I am not sure having a generic API for bypass is
> a good idea at all.
> I was thinking to totally remove it.
> Maybe we can try to have a specific API by including ixgbe_bypass.h in the
> application.
According to Jingjing's comments, I think we need not check if bypass is supported. Every API will return fail if it's not supported. I'll send a V2 to remove the bypass_is_supported.

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Ananyev, Konstantin]

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, July 11, 2016 8:11 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup
> 
> 2016-07-11 14:21, Wenzhuo Lu:
> > In testpmd code, device id is used directly to check if bypass
> > is supported. But APP should not know the details of HW, the NIC
> > specific info should not be exposed here.
> 
> Right
> 
> > This patch adds a new rte API to check if bypass is supported.
> 
> Hmmm. It's true it is cleaner. But I am not sure having a generic API
> for bypass is a good idea at all.
> I was thinking to totally remove it.

Why to remove it?
As I know there are people who use that functionality.

> Maybe we can try to have a specific API by including ixgbe_bypass.h in
> the application.

Hmm, isn't that what we were trying to get rid of in last few years?
HW specific stuff?

Konstantin

 

[dpdk-dev] [PATCH v2] app/testpmd: bypass code cleanup

[Wenzhuo Lu]

In testpmd code, device id is used directly to check if bypass
is supported. But APP should not know the details of HW, the NIC
specific info should not be exposed here.
As every bypass API does know if it's supported, no need to check
that at first. So, this patch removes the *bypass_is_supported*.

Suggested-by: Jingjing Wu <jingjing.wu@intel.com>
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 app/test-pmd/cmdline.c | 39 ---------------------------------------
 1 file changed, 39 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index b6b61ad..f90befc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -94,10 +94,6 @@ static struct cmdline *testpmd_cl;
 
 static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue);
 
-#ifdef RTE_NIC_BYPASS
-uint8_t bypass_is_supported(portid_t port_id);
-#endif
-
 /* *** Help command with introduction. *** */
 struct cmd_help_brief_result {
 	cmdline_fixed_string_t help;
@@ -3656,9 +3652,6 @@ cmd_set_bypass_mode_parsed(void *parsed_result,
 	portid_t port_id = res->port_id;
 	uint32_t bypass_mode = RTE_BYPASS_MODE_NORMAL;
 
-	if (!bypass_is_supported(port_id))
-		return;
-
 	if (!strcmp(res->value, "bypass"))
 		bypass_mode = RTE_BYPASS_MODE_BYPASS;
 	else if (!strcmp(res->value, "isolate"))
@@ -3725,9 +3718,6 @@ cmd_set_bypass_event_parsed(void *parsed_result,
 	uint32_t bypass_event = RTE_BYPASS_EVENT_NONE;
 	uint32_t bypass_mode = RTE_BYPASS_MODE_NORMAL;
 
-	if (!bypass_is_supported(port_id))
-		return;
-
 	if (!strcmp(res->event_value, "timeout"))
 		bypass_event = RTE_BYPASS_EVENT_TIMEOUT;
 	else if (!strcmp(res->event_value, "os_on"))
@@ -3903,9 +3893,6 @@ cmd_show_bypass_config_parsed(void *parsed_result,
 		"timeout"};
 	int num_events = (sizeof events) / (sizeof events[0]);
 
-	if (!bypass_is_supported(port_id))
-		return;
-
 	/* Display the bypass mode.*/
 	if (0 != rte_eth_dev_bypass_state_show(port_id, &bypass_mode)) {
 		printf("\tFailed to get bypass mode for port = %d\n", port_id);
@@ -10800,29 +10787,3 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 			ports[id].need_reconfig_queues = queue;
 	}
 }
-
-#ifdef RTE_NIC_BYPASS
-#include <rte_pci_dev_ids.h>
-uint8_t
-bypass_is_supported(portid_t port_id)
-{
-	struct rte_port   *port;
-	struct rte_pci_id *pci_id;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return 0;
-
-	/* Get the device id. */
-	port    = &ports[port_id];
-	pci_id = &port->dev_info.pci_dev->id;
-
-	/* Check if NIC supports bypass. */
-	if (pci_id->device_id == IXGBE_DEV_ID_82599_BYPASS) {
-		return 1;
-	}
-	else {
-		printf("\tBypass not supported for port_id = %d.\n", port_id);
-		return 0;
-	}
-}
-#endif
-- 
1.9.3

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Thomas Monjalon]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Hmmm. It's true it is cleaner. But I am not sure having a generic API
> > for bypass is a good idea at all.
> > I was thinking to totally remove it.
> 
> Why to remove it?
> As I know there are people who use that functionality.
> 
> > Maybe we can try to have a specific API by including ixgbe_bypass.h in
> > the application.
> 
> Hmm, isn't that what we were trying to get rid of in last few years?
> HW specific stuff?

Yes exactly.
I have the feeling the bypass API is specific to ixgbe. Isn't it?

As we will probably see other features specific to only one device.
Instead of adding a function in the generic API, I think it may be
saner to include a driver header. Then if it appears to be used
in more devices, it can be generalized.
What do you think of this approach?

Re: [dpdk-dev] [PATCH] lib/librte_ether: bypass code cleanup

[Ananyev, Konstantin]

 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > Hmmm. It's true it is cleaner. But I am not sure having a generic API
> > > for bypass is a good idea at all.
> > > I was thinking to totally remove it.
> >
> > Why to remove it?
> > As I know there are people who use that functionality.
> >
> > > Maybe we can try to have a specific API by including ixgbe_bypass.h in
> > > the application.
> >
> > Hmm, isn't that what we were trying to get rid of in last few years?
> > HW specific stuff?
> 
> Yes exactly.
> I have the feeling the bypass API is specific to ixgbe. Isn't it?

As far as I know, yes.

> 
> As we will probably see other features specific to only one device.
> Instead of adding a function in the generic API, I think it may be
> saner to include a driver header.

But that means use has to make decision based on HW id/type of the device,
the thing we were trying to get rid of in last few releases, no?

> Then if it appears to be used
> in more devices, it can be generalized.
> What do you think of this approach?

We talked few times about introducing sort of ioctl() call, to communicate
about HW specific features.
Might be a bypass I a good candidate to be moved into this ioctl() thing...
But I suppose it's too late for 16.07 to start such big changes.
If you don't like bypass API to be a generic one, my suggestion would be
to leave it as it is for 16.07, and start a discussion what it should look like
for 16.11. 

Konstantin 
 
 

Re: [dpdk-dev] specific driver API - was bypass code cleanup

[Thomas Monjalon]

2016-07-11 09:56, Ananyev, Konstantin:
> 
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > > Hmmm. It's true it is cleaner. But I am not sure having a generic API
> > > > for bypass is a good idea at all.
> > > > I was thinking to totally remove it.
> > >
> > > Why to remove it?
> > > As I know there are people who use that functionality.
> > >
> > > > Maybe we can try to have a specific API by including ixgbe_bypass.h in
> > > > the application.
> > >
> > > Hmm, isn't that what we were trying to get rid of in last few years?
> > > HW specific stuff?
> > 
> > Yes exactly.
> > I have the feeling the bypass API is specific to ixgbe. Isn't it?
> 
> As far as I know, yes.
> 
> > As we will probably see other features specific to only one device.
> > Instead of adding a function in the generic API, I think it may be
> > saner to include a driver header.
> 
> But that means use has to make decision based on HW id/type of the device,
> the thing we were trying to get rid of in last few releases, no?

Not really. If an application requires the bypass feature, we can assume
it will be used only on ixgbe NICs.
Having some generic APIs helps to deploy DPDK applications on heterogeous
machines. But if an application rely on something hardware specific, there
is no benefit of using a "fake generic layer" I guess.

> > Then if it appears to be used
> > in more devices, it can be generalized.
> > What do you think of this approach?
> 
> We talked few times about introducing sort of ioctl() call, to communicate
> about HW specific features.
> Might be a bypass I a good candidate to be moved into this ioctl() thing...

I don't see how making an ioctl-like would be better than directly including
a specific header.

> But I suppose it's too late for 16.07 to start such big changes.

Of course yes.

> If you don't like bypass API to be a generic one, my suggestion would be
> to leave it as it is for 16.07, and start a discussion what it should look like
> for 16.11.

That's what we are doing here.
I've changed the title to give a better visibility to the thread.

Re: [dpdk-dev] specific driver API - was bypass code cleanup

[Ananyev, Konstantin]

> > > > > Hmmm. It's true it is cleaner. But I am not sure having a generic API
> > > > > for bypass is a good idea at all.
> > > > > I was thinking to totally remove it.
> > > >
> > > > Why to remove it?
> > > > As I know there are people who use that functionality.
> > > >
> > > > > Maybe we can try to have a specific API by including ixgbe_bypass.h in
> > > > > the application.
> > > >
> > > > Hmm, isn't that what we were trying to get rid of in last few years?
> > > > HW specific stuff?
> > >
> > > Yes exactly.
> > > I have the feeling the bypass API is specific to ixgbe. Isn't it?
> >
> > As far as I know, yes.
> >
> > > As we will probably see other features specific to only one device.
> > > Instead of adding a function in the generic API, I think it may be
> > > saner to include a driver header.
> >
> > But that means use has to make decision based on HW id/type of the device,
> > the thing we were trying to get rid of in last few releases, no?
> 
> Not really. If an application requires the bypass feature, we can assume
> it will be used only on ixgbe NICs.

Bypass HW doesn't present on all ixgbe devices.
Only few specific models have that functionality.
So we still to provide and API for user to query is that functionality is supported or not,
user still has to make a decision based on HW id. 

> Having some generic APIs helps to deploy DPDK applications on heterogeous
> machines. But if an application rely on something hardware specific, there
> is no benefit of using a "fake generic layer" I guess.

Ok, what is your definition of 'heterogeous machines' then?
Let say today, as I know, only i40e and ixgbe can do HW mirroring of the traffic.
Is that  generic enough, or not?
I suppose we can find huge number of examples, when functionality differes
between different HW models.
As I remember that was discussed a while ago, and generic conclusion was:
avoid device specific API exposed to the application layer. 

> 
> > > Then if it appears to be used
> > > in more devices, it can be generalized.
> > > What do you think of this approach?
> >
> > We talked few times about introducing sort of ioctl() call, to communicate
> > about HW specific features.
> > Might be a bypass I a good candidate to be moved into this ioctl() thing...
> 
> I don't see how making an ioctl-like would be better than directly including
> a specific header.

User and application writer don't have to guess on what device his code will work.
He can just query what ioctl IDs that device support, and then use the supported ones.

> 
> > But I suppose it's too late for 16.07 to start such big changes.
> 
> Of course yes.

Ok, then I misunderstood you here.

> 
> > If you don't like bypass API to be a generic one, my suggestion would be
> > to leave it as it is for 16.07, and start a discussion what it should look like
> > for 16.11.
> 
> That's what we are doing here.
> I've changed the title to give a better visibility to the thread.

Ok, thanks.
Konstantin

Re: [dpdk-dev] [PATCH v2] app/testpmd: bypass code cleanup

[Thomas Monjalon]

2016-07-11 16:29, Wenzhuo Lu:
> In testpmd code, device id is used directly to check if bypass
> is supported. But APP should not know the details of HW, the NIC
> specific info should not be exposed here.
> As every bypass API does know if it's supported, no need to check
> that at first. So, this patch removes the *bypass_is_supported*.
> 
> Suggested-by: Jingjing Wu <jingjing.wu@intel.com>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Applied, thanks