[dpdk-dev,v2,2/7] ethdev: define new ethdev API rx_classification_filter_ctl
diff mbox

Message ID 1409105634-29980-3-git-send-email-jingjing.wu@intel.com
State Superseded, archived
Headers show

Commit Message

Wu, Jingjing Aug. 27, 2014, 2:13 a.m. UTC
support a new ethdev API rx_classification_filter_ctl for all
the configuration or queries for receive classification filters.
this patch supports commands the API used below:
  RTE_CMD_FDIR_RULE_ADD
  RTE_CMD_FDIR_RULE_DEL
  RTE_CMD_FDIR_FLUSH
  RTE_CMD_FDIR_INFO_GET
 
Signed-off-by: jingjing.wu <jingjing.wu@intel.com>
Reviewed-by: Helin Zhang <helin.zhang@intel.com>
Reviewed-by: Jing Chen <jing.d.chen@intel.com>
Reviewed-by: Jijiang Liu <jijiang.liu@intel.com>
 
---
 lib/librte_ether/Makefile           |   3 +-
 lib/librte_ether/rte_eth_features.h |  65 ++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c       |  19 ++++++-
 lib/librte_ether/rte_ethdev.h       | 108 +++++++++++++++++++++++-------------
 4 files changed, 155 insertions(+), 40 deletions(-)
 create mode 100644 lib/librte_ether/rte_eth_features.h

Comments

Thomas Monjalon Aug. 27, 2014, 2:22 p.m. UTC | #1
Hi Jingjing,

2014-08-27 10:13, Jingjing Wu:
> support a new ethdev API rx_classification_filter_ctl for all
> the configuration or queries for receive classification filters.
> this patch supports commands the API used below:
>   RTE_CMD_FDIR_RULE_ADD
>   RTE_CMD_FDIR_RULE_DEL
>   RTE_CMD_FDIR_FLUSH
>   RTE_CMD_FDIR_INFO_GET

Could you explain why existing API (flow director + filters) is not enough?
I'd really like to see a common API for all filtering stuff.

> -/* for 40G only */
> -#define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
> -#define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
> -#define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
> -#define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
> -#define ETH_RSS_FRAG_IPV4_SHIFT               36
> -#define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
> -#define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
> -#define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
> -#define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
> -#define ETH_RSS_FRAG_IPV6_SHIFT               46
> -#define ETH_RSS_FCOE_OX_SHIFT                 48
> -#define ETH_RSS_FCOE_RX_SHIFT                 49
> -#define ETH_RSS_FCOE_OTHER_SHIFT              50
> -#define ETH_RSS_L2_PAYLOAD_SHIFT              63
> +/* Packet Classification Type for 40G only */
> +#define ETH_PCTYPE_NONF_IPV4_UDP              31
> +#define ETH_PCTYPE_NONF_IPV4_TCP              33
> +#define ETH_PCTYPE_NONF_IPV4_SCTP             34
> +#define ETH_PCTYPE_NONF_IPV4_OTHER            35
> +#define ETH_PCTYPE_FRAG_IPV4                  36
> +#define ETH_PCTYPE_NONF_IPV6_UDP              41
> +#define ETH_PCTYPE_NONF_IPV6_TCP              43
> +#define ETH_PCTYPE_NONF_IPV6_SCTP             44
> +#define ETH_PCTYPE_NONF_IPV6_OTHER            45
> +#define ETH_PCTYPE_FRAG_IPV6                  46
> +#define ETH_PCTYPE_FCOE_OX                    48 /* not used */
> +#define ETH_PCTYPE_FCOE_RX                    49 /* not used */
> +#define ETH_PCTYPE_FCOE_OTHER                 50 /* not used */
> +#define ETH_PCTYPE_L2_PAYLOAD                 63

Why is it specific to i40e? Could we have something generic?
Please take care at having only generic things in librte_ether.

By the way, these renamings should be in a separated patch.
Wu, Jingjing Aug. 28, 2014, 3:30 a.m. UTC | #2
HI, Thomas

Just as zhang, helin's said in his pacth "[dpdk-dev] [PATCH 2/5] ethdev: add new ops of 'check_command_supported' and 'rx_classification_filter_ctl'":

'rx_classification_filter_ctl' is for receive classification filter configuring. e.g. hash function configuration, flow director configuration. It is a common API where a lot of commands can be implemented for different sub features.
We want to implement several common API for NIC specific features, to avoid creating quite a lot of ops in 'struct eth_dev_ops'. The idea came from ioctl.

Because about flow director feature, there is large gap between i40e and ixgbe. The existed flow director APIs looks specific to IXGBE, so I choose this new API to implement i40e's flow director feature.

Here, I briefly describe how the new 'rx_classification_filter_ctl' works:

The API is like below:
typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
						  enum rte_eth_command cmd,
						  void *arg);
Define a head file called rte_i40e.h in lib/librte_pmd_i40e, which contains the definition about structures specific to i40e, linked to the arg parameters above.
Define a head file called rte_eth_features.h in lib/librte_ether, which contains the commands' definition linked to cmd parameters above.
And if user want to use i40e specific features, then the head file rte_i40e.h need to be included user's application, for example, test-pmd. And user can encode these structures and call XXX_ctl API to configure their features.

Do you think it make sense?

And about the rename things, I will move it to a separate patch.


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, August 27, 2014 10:23 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API
> rx_classification_filter_ctl
> 
> Hi Jingjing,
> 
> 2014-08-27 10:13, Jingjing Wu:
> > support a new ethdev API rx_classification_filter_ctl for all
> > the configuration or queries for receive classification filters.
> > this patch supports commands the API used below:
> >   RTE_CMD_FDIR_RULE_ADD
> >   RTE_CMD_FDIR_RULE_DEL
> >   RTE_CMD_FDIR_FLUSH
> >   RTE_CMD_FDIR_INFO_GET
> 
> Could you explain why existing API (flow director + filters) is not enough?
> I'd really like to see a common API for all filtering stuff.
> 
> > -/* for 40G only */
> > -#define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
> > -#define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
> > -#define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
> > -#define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
> > -#define ETH_RSS_FRAG_IPV4_SHIFT               36
> > -#define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
> > -#define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
> > -#define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
> > -#define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
> > -#define ETH_RSS_FRAG_IPV6_SHIFT               46
> > -#define ETH_RSS_FCOE_OX_SHIFT                 48
> > -#define ETH_RSS_FCOE_RX_SHIFT                 49
> > -#define ETH_RSS_FCOE_OTHER_SHIFT              50
> > -#define ETH_RSS_L2_PAYLOAD_SHIFT              63
> > +/* Packet Classification Type for 40G only */
> > +#define ETH_PCTYPE_NONF_IPV4_UDP              31
> > +#define ETH_PCTYPE_NONF_IPV4_TCP              33
> > +#define ETH_PCTYPE_NONF_IPV4_SCTP             34
> > +#define ETH_PCTYPE_NONF_IPV4_OTHER            35
> > +#define ETH_PCTYPE_FRAG_IPV4                  36
> > +#define ETH_PCTYPE_NONF_IPV6_UDP              41
> > +#define ETH_PCTYPE_NONF_IPV6_TCP              43
> > +#define ETH_PCTYPE_NONF_IPV6_SCTP             44
> > +#define ETH_PCTYPE_NONF_IPV6_OTHER            45
> > +#define ETH_PCTYPE_FRAG_IPV6                  46
> > +#define ETH_PCTYPE_FCOE_OX                    48 /* not used */
> > +#define ETH_PCTYPE_FCOE_RX                    49 /* not used */
> > +#define ETH_PCTYPE_FCOE_OTHER                 50 /* not used */
> > +#define ETH_PCTYPE_L2_PAYLOAD                 63
> 
> Why is it specific to i40e? Could we have something generic?
> Please take care at having only generic things in librte_ether.
> 
> By the way, these renamings should be in a separated patch.
> 
> --
> Thomas
Thomas Monjalon Aug. 28, 2014, 10:55 a.m. UTC | #3
2014-08-28 03:30, Wu, Jingjing:
> We want to implement several common API for NIC specific features,
> to avoid creating quite a lot of ops in 'struct eth_dev_ops'.
> The idea came from ioctl.

The approach can be interesting.

> Because about flow director feature, there is large gap between i40e
> and ixgbe. The existed flow director APIs looks specific to IXGBE,
> so I choose this new API to implement i40e's flow director feature.

The API is not OK for you so you create another one.
I'm OK to change APIs but you should remove the old one, or at least,
implement your new API in existing drivers to allow deprecation of the
old API.
I think it would help if you start by doing ixgbe work and then apply it
to i40e.

> The API is like below:
> typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
> 						  enum rte_eth_command cmd,
> 						  void *arg);
> Define a head file called rte_i40e.h in lib/librte_pmd_i40e, which contains
> the definition about structures specific to i40e, linked to the arg
> parameters above.
> Define a head file called rte_eth_features.h in lib/librte_ether, which
> contains the commands' definition linked to cmd parameters above.

Why creating a rte_eth_features.h? Don't you think rte_ethdev.h is a good place?

> And if user want to use i40e specific features, then the head file
> rte_i40e.h need to be included user's application, for example, test-pmd.
> And user can encode these structures and call XXX_ctl API to configure
> their features.

OK, but the question is to know what is a specific feature?
I don't think flow director is a specific feature. We shouldn't have
to care if port is i40e or ixgbe to setup flow director.
Is it possible to have a common API and maybe an inheritance of the
common structure with PMD specific fields?

Example:

struct fdir_entry {
	fdir_input input;
	fdir_action action;
	enum rte_driver driver;
};
fdir_entry_generic fdir_entry = {.driver = RTE_DRIVER_GENERIC};
filter_ctl(port, FDIR_RULE_ADD, fdir_entry);

struct i40e_fdir_entry {
	struct fdir_entry generic;
	i40e_fdir_input i40e_input;
};

So i40e_input will be handled by the PMD if driver == RTE_DRIVER_I40E.

It's just an idea, comments are welcome.
Ananyev, Konstantin Aug. 28, 2014, 11:48 a.m. UTC | #4
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Thursday, August 28, 2014 11:56 AM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API rx_classification_filter_ctl
> 
> 2014-08-28 03:30, Wu, Jingjing:
> > We want to implement several common API for NIC specific features,
> > to avoid creating quite a lot of ops in 'struct eth_dev_ops'.
> > The idea came from ioctl.
> 
> The approach can be interesting.
> 
> > Because about flow director feature, there is large gap between i40e
> > and ixgbe. The existed flow director APIs looks specific to IXGBE,
> > so I choose this new API to implement i40e's flow director feature.
> 
> The API is not OK for you so you create another one.
> I'm OK to change APIs but you should remove the old one, or at least,
> implement your new API in existing drivers to allow deprecation of the
> old API.
> I think it would help if you start by doing ixgbe work and then apply it
> to i40e.
> 
> > The API is like below:
> > typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
> > 						  enum rte_eth_command cmd,
> > 						  void *arg);
> > Define a head file called rte_i40e.h in lib/librte_pmd_i40e, which contains
> > the definition about structures specific to i40e, linked to the arg
> > parameters above.
> > Define a head file called rte_eth_features.h in lib/librte_ether, which
> > contains the commands' definition linked to cmd parameters above.
> 
> Why creating a rte_eth_features.h? Don't you think rte_ethdev.h is a good place?

As I remember the long term idea was
(Jingjing please correct me, if I am wrong here):

Keep rte_ethdev.h for generic API.
Put features specific for different NICs into rte_eth_features.h 
To make things clearer and avoid polluting of rte_ethdev.h.

Provide API for the upper layer to query list of specific features(commands) supported by the underlying device.
Something like: 
rte_eth_dev_get_rx_filter_supported(port, ...);

And ioctl-type API to configure HW specific features: 
rte_eth_dev_rx_classification_filter_ctl(port, cmd, cmd_spedific_arg);

So, instead of application knows in advance "which specific NIC it is using",
application would query which features/commannds the NIC provides and then configure them appropriately.

> 
> > And if user want to use i40e specific features, then the head file
> > rte_i40e.h need to be included user's application, for example, test-pmd.
> > And user can encode these structures and call XXX_ctl API to configure
> > their features.
> 
> OK, but the question is to know what is a specific feature?
> I don't think flow director is a specific feature. We shouldn't have
> to care if port is i40e or ixgbe to setup flow director.
> Is it possible to have a common API and maybe an inheritance of the
> common structure with PMD specific fields?
> 
> Example:
> 
> struct fdir_entry {
> 	fdir_input input;
> 	fdir_action action;
> 	enum rte_driver driver;
> };
> fdir_entry_generic fdir_entry = {.driver = RTE_DRIVER_GENERIC};
> filter_ctl(port, FDIR_RULE_ADD, fdir_entry);
> 
> struct i40e_fdir_entry {
> 	struct fdir_entry generic;
> 	i40e_fdir_input i40e_input;
> };
> 
> So i40e_input will be handled by the PMD if driver == RTE_DRIVER_I40E.
> 
> It's just an idea, comments are welcome.
> 
> --
> Thomas
Wu, Jingjing Aug. 28, 2014, 1:39 p.m. UTC | #5
Hi, Thomas

Please see my comments below. 

Thanks a lot.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, August 28, 2014 6:56 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API
> rx_classification_filter_ctl
> 
> 2014-08-28 03:30, Wu, Jingjing:
> > We want to implement several common API for NIC specific features,
> > to avoid creating quite a lot of ops in 'struct eth_dev_ops'.
> > The idea came from ioctl.
> 
> The approach can be interesting.

Yes, we have discussed this approach inside. And some other Fortville
Features are also based on it, such as RSS hash function support,
mac vlan filters.
Maybe it a good chance to discuss in open forum now.

> 
> > Because about flow director feature, there is large gap between i40e
> > and ixgbe. The existed flow director APIs looks specific to IXGBE,
> > so I choose this new API to implement i40e's flow director feature.
> 
> The API is not OK for you so you create another one.
> I'm OK to change APIs but you should remove the old one, or at least,
> implement your new API in existing drivers to allow deprecation of the
> old API.
> I think it would help if you start by doing ixgbe work and then apply it
> to i40e.
> 

Yes, it will be perfect if we can use this new API to achieve flow director 
setting all types of NICs. But the concern is downward compatibility. 
Users who is planning update DPDK version need to change their code
to adapt such changes.
That's why we choose a new API instead of modifying current APIs. And 
Of course, the ideal plan is adding such XXX_ctl function in Ixgbe and
Igb to moving smoothly without removing current APIs.
I'm not sure whether I understanding correctly about the importance of
compatibility. 

> > The API is like below:
> > typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
> > 						  enum rte_eth_command cmd,
> > 						  void *arg);
> > Define a head file called rte_i40e.h in lib/librte_pmd_i40e, which contains
> > the definition about structures specific to i40e, linked to the arg
> > parameters above.
> > Define a head file called rte_eth_features.h in lib/librte_ether, which
> > contains the commands' definition linked to cmd parameters above.
> 
> Why creating a rte_eth_features.h? Don't you think rte_ethdev.h is a good place?
> 
> > And if user want to use i40e specific features, then the head file
> > rte_i40e.h need to be included user's application, for example, test-pmd.
> > And user can encode these structures and call XXX_ctl API to configure
> > their features.
> 
> OK, but the question is to know what is a specific feature?
> I don't think flow director is a specific feature. We shouldn't have
> to care if port is i40e or ixgbe to setup flow director.
> Is it possible to have a common API and maybe an inheritance of the
> common structure with PMD specific fields?
> 

Yes, flow director is not a specific feature. Even ixgbe and i40 use the same 
name. But the context and key have much difference. That's why I called it
specific.

> Example:
> 
> struct fdir_entry {
> 	fdir_input input;
> 	fdir_action action;
> 	enum rte_driver driver;
> };
> fdir_entry_generic fdir_entry = {.driver = RTE_DRIVER_GENERIC};
> filter_ctl(port, FDIR_RULE_ADD, fdir_entry);
> 
> struct i40e_fdir_entry {
> 	struct fdir_entry generic;
> 	i40e_fdir_input i40e_input;
> };
> 
> So i40e_input will be handled by the PMD if driver == RTE_DRIVER_I40E.
> 
> It's just an idea, comments are welcome.

Yes, it's a good idea about an inheritance of the common structure. I think it
may support new NIC integration in future. We can do it with the new API 
architecture. But the concern is still how to be compatible with old version.

> --
> Thomas
Wu, Jingjing Aug. 28, 2014, 2:07 p.m. UTC | #6
Hi, Konstantin

Thanks a lot.

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, August 28, 2014 7:49 PM
> To: Thomas Monjalon; Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API
> rx_classification_filter_ctl
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Thursday, August 28, 2014 11:56 AM
> > To: Wu, Jingjing
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API
> rx_classification_filter_ctl
> >
> > 2014-08-28 03:30, Wu, Jingjing:
> > > We want to implement several common API for NIC specific features,
> > > to avoid creating quite a lot of ops in 'struct eth_dev_ops'.
> > > The idea came from ioctl.
> >
> > The approach can be interesting.
> >
> > > Because about flow director feature, there is large gap between i40e
> > > and ixgbe. The existed flow director APIs looks specific to IXGBE,
> > > so I choose this new API to implement i40e's flow director feature.
> >
> > The API is not OK for you so you create another one.
> > I'm OK to change APIs but you should remove the old one, or at least,
> > implement your new API in existing drivers to allow deprecation of the
> > old API.
> > I think it would help if you start by doing ixgbe work and then apply it
> > to i40e.
> >
> > > The API is like below:
> > > typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
> > > 						  enum rte_eth_command cmd,
> > > 						  void *arg);
> > > Define a head file called rte_i40e.h in lib/librte_pmd_i40e, which contains
> > > the definition about structures specific to i40e, linked to the arg
> > > parameters above.
> > > Define a head file called rte_eth_features.h in lib/librte_ether, which
> > > contains the commands' definition linked to cmd parameters above.
> >
> > Why creating a rte_eth_features.h? Don't you think rte_ethdev.h is a good place?
> 
> As I remember the long term idea was
> (Jingjing please correct me, if I am wrong here):
> 
> Keep rte_ethdev.h for generic API.
> Put features specific for different NICs into rte_eth_features.h
> To make things clearer and avoid polluting of rte_ethdev.h.
> 
> Provide API for the upper layer to query list of specific features(commands) supported by the
> underlying device.
> Something like:
> rte_eth_dev_get_rx_filter_supported(port, ...);

Yes, in helin's patch "[dpdk-dev] [PATCH v2 2/6] ethdev: add new ops of
'is_command_supported' and 'rx_classification_filter_ctl'", he already
defined rte_eth_dev_is_command_supported. It can be used to check
whether such command can be supported by the queried port.

Actually, some features are based on this architecture, including helin's
"Support configuring hash functions" and other non-ready patch.
We supposed after any patch of ours is applied, others need to rework
then. We are using the same approach.

> And ioctl-type API to configure HW specific features:
> rte_eth_dev_rx_classification_filter_ctl(port, cmd, cmd_spedific_arg);
> 
> So, instead of application knows in advance "which specific NIC it is using",
> application would query which features/commannds the NIC provides and then configure
> them appropriately.
> 
> >
> > > And if user want to use i40e specific features, then the head file
> > > rte_i40e.h need to be included user's application, for example, test-pmd.
> > > And user can encode these structures and call XXX_ctl API to configure
> > > their features.
> >
> > OK, but the question is to know what is a specific feature?
> > I don't think flow director is a specific feature. We shouldn't have
> > to care if port is i40e or ixgbe to setup flow director.
> > Is it possible to have a common API and maybe an inheritance of the
> > common structure with PMD specific fields?
> >
> > Example:
> >
> > struct fdir_entry {
> > 	fdir_input input;
> > 	fdir_action action;
> > 	enum rte_driver driver;
> > };
> > fdir_entry_generic fdir_entry = {.driver = RTE_DRIVER_GENERIC};
> > filter_ctl(port, FDIR_RULE_ADD, fdir_entry);
> >
> > struct i40e_fdir_entry {
> > 	struct fdir_entry generic;
> > 	i40e_fdir_input i40e_input;
> > };
> >
> > So i40e_input will be handled by the PMD if driver == RTE_DRIVER_I40E.
> >
> > It's just an idea, comments are welcome.
> >
> > --
> > Thomas
Thomas Monjalon Aug. 28, 2014, 2:20 p.m. UTC | #7
2014-08-28 13:39, Wu, Jingjing:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > I'm OK to change APIs but you should remove the old one, or at least,
> > implement your new API in existing drivers to allow deprecation of the
> > old API.
> > I think it would help if you start by doing ixgbe work and then apply it
> > to i40e.
> > 
> 
> Yes, it will be perfect if we can use this new API to achieve flow director 
> setting all types of NICs. But the concern is downward compatibility.

In this case, cleanup is more important than compatibility.

> Users who is planning update DPDK version need to change their code
> to adapt such changes.

Yes, but we can keep deprecated function during 1 release.

> That's why we choose a new API instead of modifying current APIs. And 
> Of course, the ideal plan is adding such XXX_ctl function in Ixgbe and
> Igb to moving smoothly without removing current APIs.

Yes

> > I don't think flow director is a specific feature. We shouldn't have
> > to care if port is i40e or ixgbe to setup flow director.
> > Is it possible to have a common API and maybe an inheritance of the
> > common structure with PMD specific fields?
> 
> Yes, flow director is not a specific feature. Even ixgbe and i40 use the same 
> name. But the context and key have much difference. That's why I called it
> specific.
> 
> Yes, it's a good idea about an inheritance of the common structure. I think it
> may support new NIC integration in future. We can do it with the new API 
> architecture. But the concern is still how to be compatible with old version.

There is no compatibility blocker here.
If we can keep deprecated functions a while, we'll do. Otherwise, just go with
the new API.
I prefer we concentrate on good design rather than on compatibility.

Thanks
Wu, Jingjing Aug. 28, 2014, 2:31 p.m. UTC | #8
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, August 28, 2014 10:21 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 2/7] ethdev: define new ethdev API
> rx_classification_filter_ctl
> 
> 2014-08-28 13:39, Wu, Jingjing:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > I'm OK to change APIs but you should remove the old one, or at least,
> > > implement your new API in existing drivers to allow deprecation of the
> > > old API.
> > > I think it would help if you start by doing ixgbe work and then apply it
> > > to i40e.
> > >
> >
> > Yes, it will be perfect if we can use this new API to achieve flow director
> > setting all types of NICs. But the concern is downward compatibility.
> 
> In this case, cleanup is more important than compatibility.
> 
> > Users who is planning update DPDK version need to change their code
> > to adapt such changes.
> 
> Yes, but we can keep deprecated function during 1 release.
> 
> > That's why we choose a new API instead of modifying current APIs. And
> > Of course, the ideal plan is adding such XXX_ctl function in Ixgbe and
> > Igb to moving smoothly without removing current APIs.
> 
> Yes
> 
> > > I don't think flow director is a specific feature. We shouldn't have
> > > to care if port is i40e or ixgbe to setup flow director.
> > > Is it possible to have a common API and maybe an inheritance of the
> > > common structure with PMD specific fields?
> >
> > Yes, flow director is not a specific feature. Even ixgbe and i40 use the same
> > name. But the context and key have much difference. That's why I called it
> > specific.
> >
> > Yes, it's a good idea about an inheritance of the common structure. I think it
> > may support new NIC integration in future. We can do it with the new API
> > architecture. But the concern is still how to be compatible with old version.
> 
> There is no compatibility blocker here.
> If we can keep deprecated functions a while, we'll do. Otherwise, just go with
> the new API.
> I prefer we concentrate on good design rather than on compatibility.
> 

OK, now I have a rough understanding about your opinion, I guess there will be lots of rework need to be done. I will try. Thanks for your explanation.

> Thanks
> --
> Thomas

Thanks
Jingjing

Patch
diff mbox

diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
index b310f8b..03dec8a 100644
--- a/lib/librte_ether/Makefile
+++ b/lib/librte_ether/Makefile
@@ -46,8 +46,9 @@  SRCS-y += rte_ethdev.c
 #
 SYMLINK-y-include += rte_ether.h
 SYMLINK-y-include += rte_ethdev.h
+SYMLINK-y-include += rte_eth_features.h
 
 # this lib depends upon:
-DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring lib/librte_mbuf
+DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring lib/librte_mbuf lib/librte_net
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_ether/rte_eth_features.h b/lib/librte_ether/rte_eth_features.h
new file mode 100644
index 0000000..ea77b69
--- /dev/null
+++ b/lib/librte_ether/rte_eth_features.h
@@ -0,0 +1,65 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETH_FEATURES_H_
+#define _RTE_ETH_FEATURES_H_
+
+/**
+ * @file
+ *
+ * Ethernet device specific features
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Commands defined for NIC specific features */
+enum rte_eth_command {
+	RTE_CMD_UNKNOWN = 0,
+	RTE_CMD_FDIR_RULE_ADD,
+	/**< Command to add a new FDIR filter rule. */
+	RTE_CMD_FDIR_RULE_DEL,
+	/**< Command to delete a FDIR filter rule. */
+	RTE_CMD_FDIR_FLUSH,
+	/**< Command to clear all FDIR filter rules. */
+	RTE_CMD_FDIR_INFO_GET,
+	/**< Command to get FDIR information. */
+	RTE_CMD_MAX = 255,
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETH_FEATURES_H_ */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fd1010a..10a08de 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -41,7 +41,6 @@ 
 #include <errno.h>
 #include <stdint.h>
 #include <inttypes.h>
-#include <netinet/in.h>
 
 #include <rte_byteorder.h>
 #include <rte_log.h>
@@ -66,6 +65,7 @@ 
 #include <rte_errno.h>
 #include <rte_spinlock.h>
 #include <rte_string_fns.h>
+#include <rte_ip.h>
 
 #include "rte_ether.h"
 #include "rte_ethdev.h"
@@ -3002,3 +3002,20 @@  rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 	return (*dev->dev_ops->get_flex_filter)(dev, index, filter,
 						rx_queue);
 }
+
+int
+rte_eth_dev_rx_classification_filter_ctl(uint8_t port_id,
+					 enum rte_eth_command cmd,
+					 void *args)
+{
+	struct rte_eth_dev *dev;
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_classification_filter_ctl,
+								-ENOTSUP);
+	return (*dev->dev_ops->rx_classification_filter_ctl)(dev, cmd, args);
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 50df654..a2d9596 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -177,6 +177,7 @@  extern "C" {
 #include <rte_pci.h>
 #include <rte_mbuf.h>
 #include "rte_ether.h"
+#include "rte_eth_features.h"
 
 /**
  * A structure used to retrieve statistics for an Ethernet port.
@@ -345,47 +346,47 @@  struct rte_eth_rss_conf {
 #define ETH_RSS_IPV4_UDP_SHIFT                6
 #define ETH_RSS_IPV6_UDP_SHIFT                7
 #define ETH_RSS_IPV6_UDP_EX_SHIFT             8
-/* for 40G only */
-#define ETH_RSS_NONF_IPV4_UDP_SHIFT           31
-#define ETH_RSS_NONF_IPV4_TCP_SHIFT           33
-#define ETH_RSS_NONF_IPV4_SCTP_SHIFT          34
-#define ETH_RSS_NONF_IPV4_OTHER_SHIFT         35
-#define ETH_RSS_FRAG_IPV4_SHIFT               36
-#define ETH_RSS_NONF_IPV6_UDP_SHIFT           41
-#define ETH_RSS_NONF_IPV6_TCP_SHIFT           43
-#define ETH_RSS_NONF_IPV6_SCTP_SHIFT          44
-#define ETH_RSS_NONF_IPV6_OTHER_SHIFT         45
-#define ETH_RSS_FRAG_IPV6_SHIFT               46
-#define ETH_RSS_FCOE_OX_SHIFT                 48
-#define ETH_RSS_FCOE_RX_SHIFT                 49
-#define ETH_RSS_FCOE_OTHER_SHIFT              50
-#define ETH_RSS_L2_PAYLOAD_SHIFT              63
+/* Packet Classification Type for 40G only */
+#define ETH_PCTYPE_NONF_IPV4_UDP              31
+#define ETH_PCTYPE_NONF_IPV4_TCP              33
+#define ETH_PCTYPE_NONF_IPV4_SCTP             34
+#define ETH_PCTYPE_NONF_IPV4_OTHER            35
+#define ETH_PCTYPE_FRAG_IPV4                  36
+#define ETH_PCTYPE_NONF_IPV6_UDP              41
+#define ETH_PCTYPE_NONF_IPV6_TCP              43
+#define ETH_PCTYPE_NONF_IPV6_SCTP             44
+#define ETH_PCTYPE_NONF_IPV6_OTHER            45
+#define ETH_PCTYPE_FRAG_IPV6                  46
+#define ETH_PCTYPE_FCOE_OX                    48 /* not used */
+#define ETH_PCTYPE_FCOE_RX                    49 /* not used */
+#define ETH_PCTYPE_FCOE_OTHER                 50 /* not used */
+#define ETH_PCTYPE_L2_PAYLOAD                 63
 
 /* for 1G & 10G */
-#define ETH_RSS_IPV4                    ((uint16_t)1 << ETH_RSS_IPV4_SHIFT)
-#define ETH_RSS_IPV4_TCP                ((uint16_t)1 << ETH_RSS_IPV4_TCP_SHIFT)
-#define ETH_RSS_IPV6                    ((uint16_t)1 << ETH_RSS_IPV6_SHIFT)
-#define ETH_RSS_IPV6_EX                 ((uint16_t)1 << ETH_RSS_IPV6_EX_SHIFT)
-#define ETH_RSS_IPV6_TCP                ((uint16_t)1 << ETH_RSS_IPV6_TCP_SHIFT)
-#define ETH_RSS_IPV6_TCP_EX             ((uint16_t)1 << ETH_RSS_IPV6_TCP_EX_SHIFT)
-#define ETH_RSS_IPV4_UDP                ((uint16_t)1 << ETH_RSS_IPV4_UDP_SHIFT)
-#define ETH_RSS_IPV6_UDP                ((uint16_t)1 << ETH_RSS_IPV6_UDP_SHIFT)
-#define ETH_RSS_IPV6_UDP_EX             ((uint16_t)1 << ETH_RSS_IPV6_UDP_EX_SHIFT)
+#define ETH_RSS_IPV4                    (1 << ETH_RSS_IPV4_SHIFT)
+#define ETH_RSS_IPV4_TCP                (1 << ETH_RSS_IPV4_TCP_SHIFT)
+#define ETH_RSS_IPV6                    (1 << ETH_RSS_IPV6_SHIFT)
+#define ETH_RSS_IPV6_EX                 (1 << ETH_RSS_IPV6_EX_SHIFT)
+#define ETH_RSS_IPV6_TCP                (1 << ETH_RSS_IPV6_TCP_SHIFT)
+#define ETH_RSS_IPV6_TCP_EX             (1 << ETH_RSS_IPV6_TCP_EX_SHIFT)
+#define ETH_RSS_IPV4_UDP                (1 << ETH_RSS_IPV4_UDP_SHIFT)
+#define ETH_RSS_IPV6_UDP                (1 << ETH_RSS_IPV6_UDP_SHIFT)
+#define ETH_RSS_IPV6_UDP_EX             (1 << ETH_RSS_IPV6_UDP_EX_SHIFT)
 /* for 40G only */
-#define ETH_RSS_NONF_IPV4_UDP           ((uint64_t)1 << ETH_RSS_NONF_IPV4_UDP_SHIFT)
-#define ETH_RSS_NONF_IPV4_TCP           ((uint64_t)1 << ETH_RSS_NONF_IPV4_TCP_SHIFT)
-#define ETH_RSS_NONF_IPV4_SCTP          ((uint64_t)1 << ETH_RSS_NONF_IPV4_SCTP_SHIFT)
-#define ETH_RSS_NONF_IPV4_OTHER         ((uint64_t)1 << ETH_RSS_NONF_IPV4_OTHER_SHIFT)
-#define ETH_RSS_FRAG_IPV4               ((uint64_t)1 << ETH_RSS_FRAG_IPV4_SHIFT)
-#define ETH_RSS_NONF_IPV6_UDP           ((uint64_t)1 << ETH_RSS_NONF_IPV6_UDP_SHIFT)
-#define ETH_RSS_NONF_IPV6_TCP           ((uint64_t)1 << ETH_RSS_NONF_IPV6_TCP_SHIFT)
-#define ETH_RSS_NONF_IPV6_SCTP          ((uint64_t)1 << ETH_RSS_NONF_IPV6_SCTP_SHIFT)
-#define ETH_RSS_NONF_IPV6_OTHER         ((uint64_t)1 << ETH_RSS_NONF_IPV6_OTHER_SHIFT)
-#define ETH_RSS_FRAG_IPV6               ((uint64_t)1 << ETH_RSS_FRAG_IPV6_SHIFT)
-#define ETH_RSS_FCOE_OX                 ((uint64_t)1 << ETH_RSS_FCOE_OX_SHIFT) /* not used */
-#define ETH_RSS_FCOE_RX                 ((uint64_t)1 << ETH_RSS_FCOE_RX_SHIFT) /* not used */
-#define ETH_RSS_FCOE_OTHER              ((uint64_t)1 << ETH_RSS_FCOE_OTHER_SHIFT) /* not used */
-#define ETH_RSS_L2_PAYLOAD              ((uint64_t)1 << ETH_RSS_L2_PAYLOAD_SHIFT)
+#define ETH_RSS_NONF_IPV4_UDP           (1ULL << ETH_PCTYPE_NONF_IPV4_UDP)
+#define ETH_RSS_NONF_IPV4_TCP           (1ULL << ETH_PCTYPE_NONF_IPV4_TCP)
+#define ETH_RSS_NONF_IPV4_SCTP          (1ULL << ETH_PCTYPE_NONF_IPV4_SCTP)
+#define ETH_RSS_NONF_IPV4_OTHER         (1ULL << ETH_PCTYPE_NONF_IPV4_OTHER)
+#define ETH_RSS_FRAG_IPV4               (1ULL << ETH_PCTYPE_FRAG_IPV4)
+#define ETH_RSS_NONF_IPV6_UDP           (1ULL << ETH_PCTYPE_NONF_IPV6_UDP)
+#define ETH_RSS_NONF_IPV6_TCP           (1ULL << ETH_PCTYPE_NONF_IPV6_TCP)
+#define ETH_RSS_NONF_IPV6_SCTP          (1ULL << ETH_PCTYPE_NONF_IPV6_SCTP)
+#define ETH_RSS_NONF_IPV6_OTHER         (1ULL << ETH_PCTYPE_NONF_IPV6_OTHER)
+#define ETH_RSS_FRAG_IPV6               (1ULL << ETH_PCTYPE_FRAG_IPV6)
+#define ETH_RSS_FCOE_OX                 (1ULL << ETH_PCTYPE_FCOE_OX)
+#define ETH_RSS_FCOE_RX                 (1ULL << ETH_PCTYPE_FCOE_RX)
+#define ETH_RSS_FCOE_OTHER              (1ULL << ETH_PCTYPE_FCOE_OTHER)
+#define ETH_RSS_L2_PAYLOAD              (1ULL << ETH_PCTYPE_L2_PAYLOAD)
 
 #define ETH_RSS_IP ( \
 		ETH_RSS_IPV4 | \
@@ -1361,6 +1362,11 @@  typedef int (*eth_get_flex_filter_t)(struct rte_eth_dev *dev,
 			uint16_t *rx_queue);
 /**< @internal Get a flex filter rule on an Ethernet device */
 
+typedef int (*eth_rx_classification_filter_ctl_t)(struct rte_eth_dev *dev,
+						  enum rte_eth_command cmd,
+						  void *arg);
+/**< @internal receive classification filter control operations */
+
 /**
  * @internal A structure containing the functions exported by an Ethernet driver.
  */
@@ -1467,6 +1473,8 @@  struct eth_dev_ops {
 	eth_add_flex_filter_t          add_flex_filter;      /**< add flex filter. */
 	eth_remove_flex_filter_t       remove_flex_filter;   /**< remove flex filter. */
 	eth_get_flex_filter_t          get_flex_filter;      /**< get flex filter. */
+	/*common ccontrol function for receive classification filters*/
+	eth_rx_classification_filter_ctl_t rx_classification_filter_ctl;
 };
 
 /**
@@ -3557,6 +3565,30 @@  int rte_eth_dev_remove_flex_filter(uint8_t port_id, uint16_t index);
 int rte_eth_dev_get_flex_filter(uint8_t port_id, uint16_t index,
 			struct rte_flex_filter *filter, uint16_t *rx_queue);
 
+
+/**
+ * Configure the receive classification filter, including hash function
+ * selection. The commands are NIC specific in its exported public header
+ * file. Different types of NIC may have different commands.
+ * All the supported commands are defined in 'rte_eth_features.h'.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param cmd
+ *   The command.
+ * @param args
+ *   A pointer to arguments defined specifically for the command.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if <port_id> is invalid.
+ *   - others depends on the specific command implementation.
+ */
+int rte_eth_dev_rx_classification_filter_ctl(uint8_t port_id,
+					     enum rte_eth_command cmd,
+					     void *args);
+
 #ifdef __cplusplus
 }
 #endif