[dpdk-dev] lib/librte_ether: bypass code cleanup
Commit Message
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(-)
Comments
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.
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>
> -----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!
> -----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.
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
> 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?
> > 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
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.
> > > > > 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
@@ -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 {
@@ -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;
+}
@@ -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);
@@ -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,
@@ -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;
@@ -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.