[dpdk-dev,v2,12/17] net/i40e: destroy ethertype filter
Checks
Commit Message
This patch adds i40e_dev_destroy_ethertype_filter function
to destroy a ethertype filter for users.
Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
drivers/net/i40e/i40e_ethdev.c | 10 ++-------
drivers/net/i40e/i40e_ethdev.h | 5 +++++
drivers/net/i40e/i40e_flow.c | 51 ++++++++++++++++++++++++++++++++++++++++--
3 files changed, 56 insertions(+), 10 deletions(-)
Comments
>
> const struct rte_flow_ops i40e_flow_ops = {
> .validate = i40e_flow_validate,
> @@ -1492,11 +1495,16 @@ i40e_flow_destroy(__rte_unused struct
> rte_eth_dev *dev,
> struct rte_flow *flow,
> struct rte_flow_error *error)
> {
> + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> struct i40e_flow *pmd_flow = (struct i40e_flow *)flow;
> enum rte_filter_type filter_type = pmd_flow->filter_type;
> int ret;
>
> switch (filter_type) {
> + case RTE_ETH_FILTER_ETHERTYPE:
> + ret = i40e_dev_destroy_ethertype_filter(pf,
> + (struct i40e_ethertype_filter *)pmd_flow->rule);
> + break;
> default:
> PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
> filter_type);
> @@ -1504,10 +1512,49 @@ i40e_flow_destroy(__rte_unused struct
> rte_eth_dev *dev,
> break;
> }
>
> - if (ret)
> + if (!ret) {
> + TAILQ_REMOVE(&pf->flow_list, pmd_flow, node);
> + free(pmd_flow);
Should it be freed inside the function? Is the API definition like that?
On Tue, Dec 27, 2016 at 02:26:19PM +0800, Beilei Xing wrote:
> This patch adds i40e_dev_destroy_ethertype_filter function
> to destroy a ethertype filter for users.
>
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> drivers/net/i40e/i40e_ethdev.c | 10 ++-------
> drivers/net/i40e/i40e_ethdev.h | 5 +++++
> drivers/net/i40e/i40e_flow.c | 51 ++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 56 insertions(+), 10 deletions(-)
>
[...]
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> index 2a61c4f..732c411 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
[...]
> @@ -1492,11 +1495,16 @@ i40e_flow_destroy(__rte_unused struct rte_eth_dev *dev,
The `__rte_unused' qualifier should be removed.
> struct rte_flow *flow,
> struct rte_flow_error *error)
> {
> + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> struct i40e_flow *pmd_flow = (struct i40e_flow *)flow;
> enum rte_filter_type filter_type = pmd_flow->filter_type;
> int ret;
>
> switch (filter_type) {
> + case RTE_ETH_FILTER_ETHERTYPE:
> + ret = i40e_dev_destroy_ethertype_filter(pf,
> + (struct i40e_ethertype_filter *)pmd_flow->rule);
> + break;
> default:
> PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
> filter_type);
> @@ -1504,10 +1512,49 @@ i40e_flow_destroy(__rte_unused struct rte_eth_dev *dev,
> break;
> }
>
> - if (ret)
> + if (!ret) {
> + TAILQ_REMOVE(&pf->flow_list, pmd_flow, node);
> + free(pmd_flow);
> + } else {
> rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> "Failed to destroy flow.");
> + }
Probably you should introduce the pf related code when introducing
i40e_flow_destroy() in the below patch:
[PATCH v2 11/17] net/i40e: add flow destroy function
> +
> + return ret;
> +}
> +
> +static int
> +i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf,
> + struct i40e_ethertype_filter *filter)
> +{
> + struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> + struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype;
> + struct i40e_ethertype_filter *node;
> + struct i40e_control_filter_stats stats;
> + uint16_t flags = 0;
> + int ret = 0;
> +
> + if (!(filter->flags & RTE_ETHTYPE_FLAGS_MAC))
> + flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_IGNORE_MAC;
> + if (filter->flags & RTE_ETHTYPE_FLAGS_DROP)
> + flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_DROP;
> + flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_TO_QUEUE;
> +
> + memset(&stats, 0, sizeof(stats));
> + ret = i40e_aq_add_rem_control_packet_filter(hw,
> + filter->input.mac_addr.addr_bytes,
> + filter->input.ether_type,
> + flags, pf->main_vsi->seid,
> + filter->queue, 0, &stats, NULL);
> + if (ret < 0)
> + return ret;
> +
> + node = i40e_sw_ethertype_filter_lookup(ethertype_rule, &filter->input);
> + if (node)
> + ret = i40e_sw_ethertype_filter_del(pf, node);
> + else
> + return -EINVAL;
It would be more readable to check whether node equals NULL and return
when it's true, and call i40e_sw_ethertype_filter_del(pf, node) outside
the `if' statement:
node = i40e_sw_ethertype_filter_lookup(ethertype_rule, &filter->input);
if (node == NULL)
return -EINVAL;
ret = i40e_sw_ethertype_filter_del(pf, node);
Best regards,
Tiwei Bie
> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, December 28, 2016 12:56 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 12/17] net/i40e: destroy ethertype filter
>
> On Tue, Dec 27, 2016 at 02:26:19PM +0800, Beilei Xing wrote:
> > This patch adds i40e_dev_destroy_ethertype_filter function to destroy
> > a ethertype filter for users.
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev.c | 10 ++-------
> > drivers/net/i40e/i40e_ethdev.h | 5 +++++
> > drivers/net/i40e/i40e_flow.c | 51
> ++++++++++++++++++++++++++++++++++++++++--
> > 3 files changed, 56 insertions(+), 10 deletions(-)
> >
> [...]
> > diff --git a/drivers/net/i40e/i40e_flow.c
> > b/drivers/net/i40e/i40e_flow.c index 2a61c4f..732c411 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> [...]
> > @@ -1492,11 +1495,16 @@ i40e_flow_destroy(__rte_unused struct
> > rte_eth_dev *dev,
>
> The `__rte_unused' qualifier should be removed.
Yes :)
>
> > struct rte_flow *flow,
> > struct rte_flow_error *error)
> > {
> > + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> > struct i40e_flow *pmd_flow = (struct i40e_flow *)flow;
> > enum rte_filter_type filter_type = pmd_flow->filter_type;
> > int ret;
> >
> > switch (filter_type) {
> > + case RTE_ETH_FILTER_ETHERTYPE:
> > + ret = i40e_dev_destroy_ethertype_filter(pf,
> > + (struct i40e_ethertype_filter *)pmd_flow-
> >rule);
> > + break;
> > default:
> > PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
> > filter_type);
> > @@ -1504,10 +1512,49 @@ i40e_flow_destroy(__rte_unused struct
> rte_eth_dev *dev,
> > break;
> > }
> >
> > - if (ret)
> > + if (!ret) {
> > + TAILQ_REMOVE(&pf->flow_list, pmd_flow, node);
> > + free(pmd_flow);
> > + } else {
> > rte_flow_error_set(error, EINVAL,
> > RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> > "Failed to destroy flow.");
> > + }
>
> Probably you should introduce the pf related code when introducing
> i40e_flow_destroy() in the below patch:
>
> [PATCH v2 11/17] net/i40e: add flow destroy function
Good point, thanks.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf,
> > + struct i40e_ethertype_filter *filter) {
> > + struct i40e_hw *hw = I40E_PF_TO_HW(pf);
> > + struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype;
> > + struct i40e_ethertype_filter *node;
> > + struct i40e_control_filter_stats stats;
> > + uint16_t flags = 0;
> > + int ret = 0;
> > +
> > + if (!(filter->flags & RTE_ETHTYPE_FLAGS_MAC))
> > + flags |=
> I40E_AQC_ADD_CONTROL_PACKET_FLAGS_IGNORE_MAC;
> > + if (filter->flags & RTE_ETHTYPE_FLAGS_DROP)
> > + flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_DROP;
> > + flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_TO_QUEUE;
> > +
> > + memset(&stats, 0, sizeof(stats));
> > + ret = i40e_aq_add_rem_control_packet_filter(hw,
> > + filter->input.mac_addr.addr_bytes,
> > + filter->input.ether_type,
> > + flags, pf->main_vsi->seid,
> > + filter->queue, 0, &stats, NULL);
> > + if (ret < 0)
> > + return ret;
> > +
> > + node = i40e_sw_ethertype_filter_lookup(ethertype_rule, &filter-
> >input);
> > + if (node)
> > + ret = i40e_sw_ethertype_filter_del(pf, node);
> > + else
> > + return -EINVAL;
>
> It would be more readable to check whether node equals NULL and return
> when it's true, and call i40e_sw_ethertype_filter_del(pf, node) outside the
> `if' statement:
>
> node = i40e_sw_ethertype_filter_lookup(ethertype_rule, &filter-
> >input);
> if (node == NULL)
> return -EINVAL;
>
> ret = i40e_sw_ethertype_filter_del(pf, node);
Make sense, got it:)
>
> Best regards,
> Tiwei Bie
> -----Original Message-----
> From: Wu, Jingjing
> Sent: Wednesday, December 28, 2016 11:30 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 12/17] net/i40e: destroy ethertype filter
>
> >
> > const struct rte_flow_ops i40e_flow_ops = {
> > .validate = i40e_flow_validate,
> > @@ -1492,11 +1495,16 @@ i40e_flow_destroy(__rte_unused struct
> > rte_eth_dev *dev,
> > struct rte_flow *flow,
> > struct rte_flow_error *error)
> > {
> > + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
> > >dev_private);
> > struct i40e_flow *pmd_flow = (struct i40e_flow *)flow;
> > enum rte_filter_type filter_type = pmd_flow->filter_type;
> > int ret;
> >
> > switch (filter_type) {
> > + case RTE_ETH_FILTER_ETHERTYPE:
> > + ret = i40e_dev_destroy_ethertype_filter(pf,
> > + (struct i40e_ethertype_filter *)pmd_flow-
> >rule);
> > + break;
> > default:
> > PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
> > filter_type);
> > @@ -1504,10 +1512,49 @@ i40e_flow_destroy(__rte_unused struct
> > rte_eth_dev *dev,
> > break;
> > }
> >
> > - if (ret)
> > + if (!ret) {
> > + TAILQ_REMOVE(&pf->flow_list, pmd_flow, node);
> > + free(pmd_flow);
> Should it be freed inside the function? Is the API definition like that?
Yes, since API or rte won't free the flow allocated by PMD. Please refer to the attached mail.
@@ -408,14 +408,8 @@ static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
static int i40e_ethertype_filter_convert(
const struct rte_eth_ethertype_filter *input,
struct i40e_ethertype_filter *filter);
-static struct i40e_ethertype_filter *
-i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_rule *ethertype_rule,
- const struct i40e_ethertype_filter_input *input);
static int i40e_sw_ethertype_filter_insert(struct i40e_pf *pf,
struct i40e_ethertype_filter *filter);
-static int i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
- struct i40e_ethertype_filter *filter);
-
static int i40e_tunnel_filter_convert(
struct i40e_aqc_add_remove_cloud_filters_element_data *cld_filter,
struct i40e_tunnel_filter *tunnel_filter);
@@ -8191,7 +8185,7 @@ i40e_ethertype_filter_convert(const struct rte_eth_ethertype_filter *input,
}
/* Check if there exists the ehtertype filter */
-static struct i40e_ethertype_filter *
+struct i40e_ethertype_filter *
i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_rule *ethertype_rule,
const struct i40e_ethertype_filter_input *input)
{
@@ -8227,7 +8221,7 @@ i40e_sw_ethertype_filter_insert(struct i40e_pf *pf,
}
/* Delete ethertype filter in SW list */
-static int
+int
i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
struct i40e_ethertype_filter *filter)
{
@@ -776,6 +776,11 @@ int i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
int i40e_dev_tunnel_filter_set(struct i40e_pf *pf,
struct rte_eth_tunnel_filter_conf *tunnel_filter,
uint8_t add);
+struct i40e_ethertype_filter *
+i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_rule *ethertype_rule,
+ const struct i40e_ethertype_filter_input *input);
+int i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
+ struct i40e_ethertype_filter *filter);
/* I40E_DEV_PRIVATE_TO */
#define I40E_DEV_PRIVATE_TO_PF(adapter) \
@@ -49,6 +49,7 @@
#include "i40e_logs.h"
#include "base/i40e_type.h"
+#include "base/i40e_prototype.h"
#include "i40e_ethdev.h"
#define I40E_IPV4_TC_SHIFT 4
@@ -67,7 +68,7 @@ static struct rte_flow *i40e_flow_create(struct rte_eth_dev *dev,
const struct rte_flow_item pattern[],
const struct rte_flow_action actions[],
struct rte_flow_error *error);
-static int i40e_flow_destroy(__rte_unused struct rte_eth_dev *dev,
+static int i40e_flow_destroy(struct rte_eth_dev *dev,
struct rte_flow *flow,
struct rte_flow_error *error);
static int i40e_parse_ethertype_pattern(const struct rte_flow_item *pattern,
@@ -90,6 +91,8 @@ static int i40e_parse_tunnel_act(const struct rte_flow_action *actions,
struct rte_eth_tunnel_filter_conf *filter);
static int i40e_parse_attr(const struct rte_flow_attr *attr,
struct rte_flow_error *error);
+static int i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf,
+ struct i40e_ethertype_filter *filter);
const struct rte_flow_ops i40e_flow_ops = {
.validate = i40e_flow_validate,
@@ -1492,11 +1495,16 @@ i40e_flow_destroy(__rte_unused struct rte_eth_dev *dev,
struct rte_flow *flow,
struct rte_flow_error *error)
{
+ struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
struct i40e_flow *pmd_flow = (struct i40e_flow *)flow;
enum rte_filter_type filter_type = pmd_flow->filter_type;
int ret;
switch (filter_type) {
+ case RTE_ETH_FILTER_ETHERTYPE:
+ ret = i40e_dev_destroy_ethertype_filter(pf,
+ (struct i40e_ethertype_filter *)pmd_flow->rule);
+ break;
default:
PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
filter_type);
@@ -1504,10 +1512,49 @@ i40e_flow_destroy(__rte_unused struct rte_eth_dev *dev,
break;
}
- if (ret)
+ if (!ret) {
+ TAILQ_REMOVE(&pf->flow_list, pmd_flow, node);
+ free(pmd_flow);
+ } else {
rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
"Failed to destroy flow.");
+ }
+
+ return ret;
+}
+
+static int
+i40e_dev_destroy_ethertype_filter(struct i40e_pf *pf,
+ struct i40e_ethertype_filter *filter)
+{
+ struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+ struct i40e_ethertype_rule *ethertype_rule = &pf->ethertype;
+ struct i40e_ethertype_filter *node;
+ struct i40e_control_filter_stats stats;
+ uint16_t flags = 0;
+ int ret = 0;
+
+ if (!(filter->flags & RTE_ETHTYPE_FLAGS_MAC))
+ flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_IGNORE_MAC;
+ if (filter->flags & RTE_ETHTYPE_FLAGS_DROP)
+ flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_DROP;
+ flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_TO_QUEUE;
+
+ memset(&stats, 0, sizeof(stats));
+ ret = i40e_aq_add_rem_control_packet_filter(hw,
+ filter->input.mac_addr.addr_bytes,
+ filter->input.ether_type,
+ flags, pf->main_vsi->seid,
+ filter->queue, 0, &stats, NULL);
+ if (ret < 0)
+ return ret;
+
+ node = i40e_sw_ethertype_filter_lookup(ethertype_rule, &filter->input);
+ if (node)
+ ret = i40e_sw_ethertype_filter_del(pf, node);
+ else
+ return -EINVAL;
return ret;
}