* Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
From: Jiri Pirko @ 2018-11-19 12:16 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-5-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:11AM CET, pablo@netfilter.org wrote:
>This patch implements a new function to translate from native TC action
>to the new flow_action representation. Moreover, this patch also updates
>cls_flower to use this new function.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> include/net/pkt_cls.h | 3 ++
> net/sched/cls_api.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/sched/cls_flower.c | 15 ++++++-
> 3 files changed, 130 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 8b79a1a3a5c7..7d7aefa5fcd2 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -619,6 +619,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> }
> #endif /* CONFIG_NET_CLS_IND */
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts);
>+
> int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> enum tc_setup_type type, void *type_data, bool err_stop);
>
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index d92f44ac4c39..6ab44e650f43 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -31,6 +31,14 @@
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> #include <net/pkt_cls.h>
>+#include <net/tc_act/tc_mirred.h>
>+#include <net/tc_act/tc_vlan.h>
>+#include <net/tc_act/tc_tunnel_key.h>
>+#include <net/tc_act/tc_pedit.h>
>+#include <net/tc_act/tc_csum.h>
>+#include <net/tc_act/tc_gact.h>
>+#include <net/tc_act/tc_skbedit.h>
>+#include <net/tc_act/tc_mirred.h>
>
> extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
>
>@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> }
> EXPORT_SYMBOL(tc_setup_cb_call);
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts)
>+{
>+ const struct tc_action *act;
>+ int num_acts = 0, i, j, k;
>+
>+ if (!exts)
>+ return 0;
>+
>+ tcf_exts_for_each_action(i, act, exts) {
>+ if (is_tcf_pedit(act))
>+ num_acts += tcf_pedit_nkeys(act);
>+ else
>+ num_acts++;
>+ }
>+ if (!num_acts)
>+ return 0;
>+
>+ if (flow_action_init(flow_action, num_acts) < 0)
>+ return -ENOMEM;
>+
>+ j = 0;
>+ tcf_exts_for_each_action(i, act, exts) {
>+ struct flow_action_key *key;
>+
>+ key = &flow_action->keys[j];
>+ if (is_tcf_gact_ok(act)) {
>+ key->id = FLOW_ACTION_KEY_ACCEPT;
>+ } else if (is_tcf_gact_shot(act)) {
>+ key->id = FLOW_ACTION_KEY_DROP;
>+ } else if (is_tcf_gact_trap(act)) {
>+ key->id = FLOW_ACTION_KEY_TRAP;
>+ } else if (is_tcf_gact_goto_chain(act)) {
>+ key->id = FLOW_ACTION_KEY_GOTO;
>+ key->chain_index = tcf_gact_goto_chain_index(act);
>+ } else if (is_tcf_mirred_egress_redirect(act)) {
>+ key->id = FLOW_ACTION_KEY_REDIRECT;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_mirred_egress_mirror(act)) {
>+ key->id = FLOW_ACTION_KEY_MIRRED;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_vlan(act)) {
>+ switch (tcf_vlan_action(act)) {
>+ case TCA_VLAN_ACT_PUSH:
>+ key->id = FLOW_ACTION_KEY_VLAN_PUSH;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ case TCA_VLAN_ACT_POP:
>+ key->id = FLOW_ACTION_KEY_VLAN_POP;
>+ break;
>+ case TCA_VLAN_ACT_MODIFY:
>+ key->id = FLOW_ACTION_KEY_VLAN_MANGLE;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ default:
>+ goto err_out;
>+ }
>+ } else if (is_tcf_tunnel_set(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_ENCAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_tunnel_release(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_DECAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_pedit(act)) {
>+ for (k = 0; k < tcf_pedit_nkeys(act); k++) {
>+ switch (tcf_pedit_cmd(act, k)) {
>+ case TCA_PEDIT_KEY_EX_CMD_SET:
>+ key->id = FLOW_ACTION_KEY_MANGLE;
>+ break;
>+ case TCA_PEDIT_KEY_EX_CMD_ADD:
>+ key->id = FLOW_ACTION_KEY_ADD;
>+ break;
>+ default:
>+ goto err_out;
>+ }
>+ key->mangle.htype = tcf_pedit_htype(act, k);
>+ key->mangle.mask = tcf_pedit_mask(act, k);
>+ key->mangle.val = tcf_pedit_val(act, k);
>+ key->mangle.offset = tcf_pedit_offset(act, k);
>+ key = &flow_action->keys[++j];
>+ }
>+ } else if (is_tcf_csum(act)) {
>+ key->id = FLOW_ACTION_KEY_CSUM;
>+ key->csum_flags = tcf_csum_update_flags(act);
>+ } else if (is_tcf_skbedit_mark(act)) {
>+ key->id = FLOW_ACTION_KEY_MARK;
>+ key->mark = tcf_skbedit_mark(act);
>+ } else {
>+ goto err_out;
>+ }
>+
>+ if (!is_tcf_pedit(act))
>+ j++;
>+ }
>+ return 0;
>+err_out:
>+ flow_action_free(flow_action);
>+ return -EOPNOTSUPP;
>+}
>+EXPORT_SYMBOL(tc_setup_flow_action);
>+
> static __net_init int tcf_net_init(struct net *net)
> {
> struct tcf_net *tn = net_generic(net, tcf_net_id);
>diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>index 26fc129ed504..a301fb8e68e7 100644
>--- a/net/sched/cls_flower.c
>+++ b/net/sched/cls_flower.c
>@@ -104,6 +104,7 @@ struct cls_fl_filter {
> u32 in_hw_count;
> struct rcu_work rwork;
> struct net_device *hw_dev;
>+ struct flow_action action;
> };
>
> static const struct rhashtable_params mask_ht_params = {
>@@ -391,18 +392,27 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
> cls_flower.exts = &f->exts;
> cls_flower.classid = f->res.classid;
>
>+ if (tc_setup_flow_action(&f->action, &f->exts) < 0)
>+ return -ENOMEM;
>+
>+ cls_flower.rule.action.keys = f->action.keys;
>+ cls_flower.rule.action.num_keys = f->action.num_keys;
Hmm, I think flow actions should be only field in rule. Flower does not
use it internally, so it does not really make sense to have f->action
>+
> err = tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
> &cls_flower, skip_sw);
> if (err < 0) {
> fl_hw_destroy_filter(tp, f, NULL);
>+ flow_action_free(&f->action);
> return err;
> } else if (err > 0) {
> f->in_hw_count = err;
> tcf_block_offload_inc(block, &f->flags);
> }
>
>- if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW))
>+ if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW)) {
>+ flow_action_free(&f->action);
> return -EINVAL;
>+ }
>
> return 0;
> }
>@@ -429,6 +439,7 @@ static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> bool async = tcf_exts_get_net(&f->exts);
> bool last;
>
>+ flow_action_free(&f->action);
> idr_remove(&head->handle_idr, f->handle);
> list_del_rcu(&f->list);
> last = fl_mask_put(head, f->mask, async);
>@@ -1470,6 +1481,8 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
> cls_flower.rule.match.mask = &mask->key;
> cls_flower.rule.match.key = &f->mkey;
> cls_flower.exts = &f->exts;
>+ cls_flower.rule.action.num_keys = f->action.num_keys;
>+ cls_flower.rule.action.keys = f->action.keys;
> cls_flower.classid = f->res.classid;
>
> err = cb(TC_SETUP_CLSFLOWER, &cls_flower, cb_priv);
>--
>2.11.0
>
^ permalink raw reply
* [PATCH net-next] net: hns3: add common validation in hclge_dcb
From: Tan Xiaojun @ 2018-11-19 13:02 UTC (permalink / raw)
To: yisen.zhuang, salil.mehta, davem, lipeng321, linyunsheng,
shenjian15, tanhuazhong, liangfuyun1
Cc: netdev, linux-kernel
From: Yunsheng Lin <linyunsheng@huawei.com>
Before setting tm related configuration to hardware, driver
needs to check the configuration provided by user is valid.
Currently hclge_ieee_setets and hclge_setup_tc both implement
their own checking, which has a lot in common.
This patch addes hclge_dcb_common_validate to do the common
checking. The checking in hclge_tm_prio_tc_info_update
and hclge_tm_schd_info_update is unnecessary now, so change
the return type to void, which removes the need to do error
handling when one of the checking fails.
Also, ets->prio_tc is indexed by user prio and ets->tc_tsa is
indexed by tc num, so this patch changes them to use different
index.
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
---
.../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 70 +++++++++++++++-------
.../net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c | 14 +----
.../net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h | 4 +-
3 files changed, 53 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
index e72f724..f6323b2 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
@@ -35,7 +35,9 @@ static int hclge_ieee_ets_to_tm_info(struct hclge_dev *hdev,
}
}
- return hclge_tm_prio_tc_info_update(hdev, ets->prio_tc);
+ hclge_tm_prio_tc_info_update(hdev, ets->prio_tc);
+
+ return 0;
}
static void hclge_tm_info_to_ieee_ets(struct hclge_dev *hdev,
@@ -70,25 +72,61 @@ static int hclge_ieee_getets(struct hnae3_handle *h, struct ieee_ets *ets)
return 0;
}
+static int hclge_dcb_common_validate(struct hclge_dev *hdev, u8 num_tc,
+ u8 *prio_tc)
+{
+ int i;
+
+ if (num_tc > hdev->tc_max) {
+ dev_err(&hdev->pdev->dev,
+ "tc num checking failed, %u > tc_max(%u)\n",
+ num_tc, hdev->tc_max);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < HNAE3_MAX_USER_PRIO; i++) {
+ if (prio_tc[i] >= num_tc) {
+ dev_err(&hdev->pdev->dev,
+ "prio_tc[%u] checking failed, %u >= num_tc(%u)\n",
+ i, prio_tc[i], num_tc);
+ return -EINVAL;
+ }
+ }
+
+ for (i = 0; i < hdev->num_alloc_vport; i++) {
+ if (num_tc > hdev->vport[i].alloc_tqps) {
+ dev_err(&hdev->pdev->dev,
+ "allocated tqp(%u) checking failed, %u > tqp(%u)\n",
+ i, num_tc, hdev->vport[i].alloc_tqps);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
static int hclge_ets_validate(struct hclge_dev *hdev, struct ieee_ets *ets,
u8 *tc, bool *changed)
{
bool has_ets_tc = false;
u32 total_ets_bw = 0;
u8 max_tc = 0;
+ int ret;
u8 i;
- for (i = 0; i < HNAE3_MAX_TC; i++) {
- if (ets->prio_tc[i] >= hdev->tc_max ||
- i >= hdev->tc_max)
- return -EINVAL;
-
+ for (i = 0; i < HNAE3_MAX_USER_PRIO; i++) {
if (ets->prio_tc[i] != hdev->tm_info.prio_tc[i])
*changed = true;
if (ets->prio_tc[i] > max_tc)
max_tc = ets->prio_tc[i];
+ }
+ ret = hclge_dcb_common_validate(hdev, max_tc + 1, ets->prio_tc);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < HNAE3_MAX_TC; i++) {
switch (ets->tc_tsa[i]) {
case IEEE_8021QAZ_TSA_STRICT:
if (hdev->tm_info.tc_info[i].tc_sch_mode !=
@@ -184,9 +222,7 @@ static int hclge_ieee_setets(struct hnae3_handle *h, struct ieee_ets *ets)
if (ret)
return ret;
- ret = hclge_tm_schd_info_update(hdev, num_tc);
- if (ret)
- return ret;
+ hclge_tm_schd_info_update(hdev, num_tc);
ret = hclge_ieee_ets_to_tm_info(hdev, ets);
if (ret)
@@ -305,20 +341,12 @@ static int hclge_setup_tc(struct hnae3_handle *h, u8 tc, u8 *prio_tc)
if (hdev->flag & HCLGE_FLAG_DCB_ENABLE)
return -EINVAL;
- if (tc > hdev->tc_max) {
- dev_err(&hdev->pdev->dev,
- "setup tc failed, tc(%u) > tc_max(%u)\n",
- tc, hdev->tc_max);
- return -EINVAL;
- }
-
- ret = hclge_tm_schd_info_update(hdev, tc);
+ ret = hclge_dcb_common_validate(hdev, tc, prio_tc);
if (ret)
- return ret;
+ return -EINVAL;
- ret = hclge_tm_prio_tc_info_update(hdev, prio_tc);
- if (ret)
- return ret;
+ hclge_tm_schd_info_update(hdev, tc);
+ hclge_tm_prio_tc_info_update(hdev, prio_tc);
ret = hclge_tm_init_hw(hdev);
if (ret)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
index 494e562..00458da 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
@@ -1259,15 +1259,13 @@ int hclge_pause_setup_hw(struct hclge_dev *hdev)
return 0;
}
-int hclge_tm_prio_tc_info_update(struct hclge_dev *hdev, u8 *prio_tc)
+void hclge_tm_prio_tc_info_update(struct hclge_dev *hdev, u8 *prio_tc)
{
struct hclge_vport *vport = hdev->vport;
struct hnae3_knic_private_info *kinfo;
u32 i, k;
for (i = 0; i < HNAE3_MAX_USER_PRIO; i++) {
- if (prio_tc[i] >= hdev->tm_info.num_tc)
- return -EINVAL;
hdev->tm_info.prio_tc[i] = prio_tc[i];
for (k = 0; k < hdev->num_alloc_vport; k++) {
@@ -1275,18 +1273,12 @@ int hclge_tm_prio_tc_info_update(struct hclge_dev *hdev, u8 *prio_tc)
kinfo->prio_tc[i] = prio_tc[i];
}
}
- return 0;
}
-int hclge_tm_schd_info_update(struct hclge_dev *hdev, u8 num_tc)
+void hclge_tm_schd_info_update(struct hclge_dev *hdev, u8 num_tc)
{
u8 i, bit_map = 0;
- for (i = 0; i < hdev->num_alloc_vport; i++) {
- if (num_tc > hdev->vport[i].alloc_tqps)
- return -EINVAL;
- }
-
hdev->tm_info.num_tc = num_tc;
for (i = 0; i < hdev->tm_info.num_tc; i++)
@@ -1300,8 +1292,6 @@ int hclge_tm_schd_info_update(struct hclge_dev *hdev, u8 num_tc)
hdev->hw_tc_map = bit_map;
hclge_tm_schd_info_init(hdev);
-
- return 0;
}
int hclge_tm_init_hw(struct hclge_dev *hdev)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
index 25eef13..4bd916a 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
@@ -131,8 +131,8 @@ struct hclge_port_shapping_cmd {
int hclge_tm_schd_init(struct hclge_dev *hdev);
int hclge_pause_setup_hw(struct hclge_dev *hdev);
int hclge_tm_schd_mode_hw(struct hclge_dev *hdev);
-int hclge_tm_prio_tc_info_update(struct hclge_dev *hdev, u8 *prio_tc);
-int hclge_tm_schd_info_update(struct hclge_dev *hdev, u8 num_tc);
+void hclge_tm_prio_tc_info_update(struct hclge_dev *hdev, u8 *prio_tc);
+void hclge_tm_schd_info_update(struct hclge_dev *hdev, u8 num_tc);
int hclge_tm_dwrr_cfg(struct hclge_dev *hdev);
int hclge_tm_map_cfg(struct hclge_dev *hdev);
int hclge_tm_init_hw(struct hclge_dev *hdev);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure
From: Pablo Neira Ayuso @ 2018-11-19 12:35 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119115623.GA2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 12:56:23PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:10AM CET, pablo@netfilter.org wrote:
> >This new infrastructure defines the nic actions that you can perform
> >from existing network drivers. This infrastructure allows us to avoid a
> >direct dependency with the native software TC action representation.
> >
> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >---
> >v2: no changes.
> >
> > include/net/flow_dissector.h | 70 ++++++++++++++++++++++++++++++++++++++++++++
> > net/core/flow_dissector.c | 18 ++++++++++++
> > 2 files changed, 88 insertions(+)
> >
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index 965a82b8d881..925c208816f1 100644
> >--- a/include/net/flow_dissector.h
> >+++ b/include/net/flow_dissector.h
> >@@ -402,8 +402,78 @@ void flow_rule_match_enc_keyid(const struct flow_rule *rule,
> > void flow_rule_match_enc_opts(const struct flow_rule *rule,
> > struct flow_match_enc_opts *out);
> >
> >+enum flow_action_key_id {
>
> Why "key"? Why not just "flow_action_id"
Sure, will rename this.
> >+ FLOW_ACTION_KEY_ACCEPT = 0,
> >+ FLOW_ACTION_KEY_DROP,
> >+ FLOW_ACTION_KEY_TRAP,
> >+ FLOW_ACTION_KEY_GOTO,
> >+ FLOW_ACTION_KEY_REDIRECT,
> >+ FLOW_ACTION_KEY_MIRRED,
> >+ FLOW_ACTION_KEY_VLAN_PUSH,
> >+ FLOW_ACTION_KEY_VLAN_POP,
> >+ FLOW_ACTION_KEY_VLAN_MANGLE,
> >+ FLOW_ACTION_KEY_TUNNEL_ENCAP,
> >+ FLOW_ACTION_KEY_TUNNEL_DECAP,
> >+ FLOW_ACTION_KEY_MANGLE,
> >+ FLOW_ACTION_KEY_ADD,
> >+ FLOW_ACTION_KEY_CSUM,
> >+ FLOW_ACTION_KEY_MARK,
I assume I should remove _KEY_ from this enum definitions too.
> >+};
> >+
> >+/* This is mirroring enum pedit_header_type definition for easy mapping between
> >+ * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
> >+ * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
> >+ */
> >+enum flow_act_mangle_base {
>
> Please be consistent in naming: "act" vs "action"
OK.
> >+ FLOW_ACT_MANGLE_UNSPEC = 0,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_ETH,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP4,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP6,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_TCP,
> >+ FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >+};
> >+
> >+struct flow_action_key {
>
> And here "struct flow_action"
OK.
> >+ enum flow_action_key_id id;
> >+ union {
> >+ u32 chain_index; /* FLOW_ACTION_KEY_GOTO */
> >+ struct net_device *dev; /* FLOW_ACTION_KEY_REDIRECT */
> >+ struct { /* FLOW_ACTION_KEY_VLAN */
> >+ u16 vid;
> >+ __be16 proto;
> >+ u8 prio;
> >+ } vlan;
> >+ struct { /* FLOW_ACTION_KEY_PACKET_EDIT */
> >+ enum flow_act_mangle_base htype;
> >+ u32 offset;
> >+ u32 mask;
> >+ u32 val;
> >+ } mangle;
> >+ const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
> >+ u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
> >+ u32 mark; /* FLOW_ACTION_KEY_MARK */
> >+ };
> >+};
> >+
> >+struct flow_action {
>
> And here "struct flow_actions"
>
>
> >+ int num_keys;
>
> unsigned int;
OK.
^ permalink raw reply
* Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
From: Pablo Neira Ayuso @ 2018-11-19 12:37 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119121630.GD2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 01:16:30PM +0100, Jiri Pirko wrote:
> >@@ -391,18 +392,27 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
> > cls_flower.exts = &f->exts;
> > cls_flower.classid = f->res.classid;
> >
> >+ if (tc_setup_flow_action(&f->action, &f->exts) < 0)
> >+ return -ENOMEM;
> >+
> >+ cls_flower.rule.action.keys = f->action.keys;
> >+ cls_flower.rule.action.num_keys = f->action.num_keys;
>
> Hmm, I think flow actions should be only field in rule. Flower does not
> use it internally, so it does not really make sense to have f->action
OK, will remove this new field from flower.
Thanks!
^ permalink raw reply
* Re: [PATCH 1/4] net-next/hinic:replace multiply and division operators
From: David Miller @ 2018-11-19 23:01 UTC (permalink / raw)
To: xuechaojing
Cc: linux-kernel, netdev, wulike1, chiqijun, fy.wang, tony.qu,
luoshaokai
In-Reply-To: <20181119061234.12839-1-xuechaojing@huawei.com>
From: Xue Chaojing <xuechaojing@huawei.com>
Date: Mon, 19 Nov 2018 06:12:31 +0000
> @@ -530,7 +536,9 @@ int hinic_wq_allocate(struct hinic_wqs *wqs, struct hinic_wq *wq,
> return -EINVAL;
> }
>
> - num_wqebbs_per_page = ALIGN(wq_page_size, wqebb_size) / wqebb_size;
> + wqebb_size_shift = ilog2(wqebb_size);
You now have introduced the assumption that these various sizes are a power
of two.
You should check for this, either at compile time or at run time, in
order to avoid surprises and hard to debug issues in the future.
Thank you.
^ permalink raw reply
* [PATCH v2 1/5] usb: split code locating ACPI companion into port and device
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
linux-bluetooth, linux-kernel, linux-usb, netdev
Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan,
sukumar.ghorai
In-Reply-To: <20181117010748.24347-1-rajatja@google.com>
From: Dmitry Torokhov <dtor@chromium.org>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
linux-bluetooth, linux-kernel, linux-usb, netdev
Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan,
sukumar.ghorai
In-Reply-To: <20181119230409.258121-1-rajatja@google.com>
From: Dmitry Torokhov <dtor@chromium.org>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
---
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH v2 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
linux-bluetooth, linux-kernel, linux-usb, netdev
Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan,
sukumar.ghorai
In-Reply-To: <20181119230409.258121-1-rajatja@google.com>
If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Handle the EPROBE_DEFER case.
drivers/bluetooth/btusb.c | 42 +++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e8e148480c91..bf522cfe68c1 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -29,6 +29,7 @@
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/suspend.h>
+#include <linux/gpio/consumer.h>
#include <asm/unaligned.h>
#include <net/bluetooth/bluetooth.h>
@@ -475,6 +476,8 @@ struct btusb_data {
struct usb_endpoint_descriptor *diag_tx_ep;
struct usb_endpoint_descriptor *diag_rx_ep;
+ struct gpio_desc *reset_gpio;
+
__u8 cmdreq_type;
__u8 cmdreq;
@@ -490,6 +493,28 @@ struct btusb_data {
int oob_wake_irq; /* irq for out-of-band wake-on-bt */
};
+
+static void btusb_hw_reset(struct hci_dev *hdev)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct gpio_desc *reset_gpio = data->reset_gpio;
+
+ /*
+ * Toggle the hard reset line if the platform provides one. The reset
+ * is going to yank the device off the USB and then replug. So doing
+ * once is enough. The cleanup is handled correctly on the way out
+ * (standard USB disconnect), and the new device is detected cleanly
+ * and bound to the driver again like it should be.
+ */
+ if (reset_gpio) {
+ bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);
+ clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
+ gpiod_set_value(reset_gpio, 1);
+ mdelay(100);
+ gpiod_set_value(reset_gpio, 0);
+ }
+}
+
static inline void btusb_free_frags(struct btusb_data *data)
{
unsigned long flags;
@@ -2917,6 +2942,7 @@ static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct usb_endpoint_descriptor *ep_desc;
+ struct gpio_desc *reset_gpio;
struct btusb_data *data;
struct hci_dev *hdev;
unsigned ifnum_base;
@@ -3030,6 +3056,16 @@ static int btusb_probe(struct usb_interface *intf,
SET_HCIDEV_DEV(hdev, &intf->dev);
+ reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
+ GPIOD_OUT_LOW);
+ if (PTR_ERR(reset_gpio) == -EPROBE_DEFER) {
+ err = -EPROBE_DEFER;
+ goto out_free_dev;
+ } else if (!IS_ERR(reset_gpio)) {
+ data->reset_gpio = reset_gpio;
+ hdev->hw_reset = btusb_hw_reset;
+ }
+
hdev->open = btusb_open;
hdev->close = btusb_close;
hdev->flush = btusb_flush;
@@ -3085,6 +3121,7 @@ static int btusb_probe(struct usb_interface *intf,
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
+ set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
if (id->driver_info & BTUSB_INTEL) {
hdev->setup = btusb_setup_intel;
@@ -3225,6 +3262,8 @@ static int btusb_probe(struct usb_interface *intf,
return 0;
out_free_dev:
+ if (data->reset_gpio)
+ gpiod_put(data->reset_gpio);
hci_free_dev(hdev);
return err;
}
@@ -3268,6 +3307,9 @@ static void btusb_disconnect(struct usb_interface *intf)
if (data->oob_wake_irq)
device_init_wakeup(&data->udev->dev, false);
+ if (data->reset_gpio)
+ gpiod_put(data->reset_gpio);
+
hci_free_dev(hdev);
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure
From: Jiri Pirko @ 2018-11-19 13:05 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119123548.qk74xgntgfp6msqm@salvia>
Mon, Nov 19, 2018 at 01:35:48PM CET, pablo@netfilter.org wrote:
>On Mon, Nov 19, 2018 at 12:56:23PM +0100, Jiri Pirko wrote:
>> Mon, Nov 19, 2018 at 01:15:10AM CET, pablo@netfilter.org wrote:
>> >This new infrastructure defines the nic actions that you can perform
>> >from existing network drivers. This infrastructure allows us to avoid a
>> >direct dependency with the native software TC action representation.
>> >
>> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> >---
>> >v2: no changes.
>> >
>> > include/net/flow_dissector.h | 70 ++++++++++++++++++++++++++++++++++++++++++++
>> > net/core/flow_dissector.c | 18 ++++++++++++
>> > 2 files changed, 88 insertions(+)
>> >
>> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>> >index 965a82b8d881..925c208816f1 100644
>> >--- a/include/net/flow_dissector.h
>> >+++ b/include/net/flow_dissector.h
>> >@@ -402,8 +402,78 @@ void flow_rule_match_enc_keyid(const struct flow_rule *rule,
>> > void flow_rule_match_enc_opts(const struct flow_rule *rule,
>> > struct flow_match_enc_opts *out);
>> >
>> >+enum flow_action_key_id {
>>
>> Why "key"? Why not just "flow_action_id"
>
>Sure, will rename this.
>
>> >+ FLOW_ACTION_KEY_ACCEPT = 0,
>> >+ FLOW_ACTION_KEY_DROP,
>> >+ FLOW_ACTION_KEY_TRAP,
>> >+ FLOW_ACTION_KEY_GOTO,
>> >+ FLOW_ACTION_KEY_REDIRECT,
>> >+ FLOW_ACTION_KEY_MIRRED,
>> >+ FLOW_ACTION_KEY_VLAN_PUSH,
>> >+ FLOW_ACTION_KEY_VLAN_POP,
>> >+ FLOW_ACTION_KEY_VLAN_MANGLE,
>> >+ FLOW_ACTION_KEY_TUNNEL_ENCAP,
>> >+ FLOW_ACTION_KEY_TUNNEL_DECAP,
>> >+ FLOW_ACTION_KEY_MANGLE,
>> >+ FLOW_ACTION_KEY_ADD,
>> >+ FLOW_ACTION_KEY_CSUM,
>> >+ FLOW_ACTION_KEY_MARK,
>
>I assume I should remove _KEY_ from this enum definitions too.
Sure.
>
>> >+};
>> >+
>> >+/* This is mirroring enum pedit_header_type definition for easy mapping between
>> >+ * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
>> >+ * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
>> >+ */
>> >+enum flow_act_mangle_base {
>>
>> Please be consistent in naming: "act" vs "action"
>
>OK.
>
>> >+ FLOW_ACT_MANGLE_UNSPEC = 0,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_ETH,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP4,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP6,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_TCP,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>> >+};
>> >+
>> >+struct flow_action_key {
>>
>> And here "struct flow_action"
>
>OK.
>
>> >+ enum flow_action_key_id id;
>> >+ union {
>> >+ u32 chain_index; /* FLOW_ACTION_KEY_GOTO */
>> >+ struct net_device *dev; /* FLOW_ACTION_KEY_REDIRECT */
>> >+ struct { /* FLOW_ACTION_KEY_VLAN */
>> >+ u16 vid;
>> >+ __be16 proto;
>> >+ u8 prio;
>> >+ } vlan;
>> >+ struct { /* FLOW_ACTION_KEY_PACKET_EDIT */
>> >+ enum flow_act_mangle_base htype;
>> >+ u32 offset;
>> >+ u32 mask;
>> >+ u32 val;
>> >+ } mangle;
>> >+ const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
>> >+ u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
>> >+ u32 mark; /* FLOW_ACTION_KEY_MARK */
>> >+ };
>> >+};
>> >+
>> >+struct flow_action {
>>
>> And here "struct flow_actions"
>>
>>
>> >+ int num_keys;
>>
>> unsigned int;
>
>OK.
^ permalink raw reply
* Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
From: Pablo Neira Ayuso @ 2018-11-19 13:21 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119121251.GC2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 01:12:51PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:11AM CET, pablo@netfilter.org wrote:
> >@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> > }
> > EXPORT_SYMBOL(tc_setup_cb_call);
> >
> >+int tc_setup_flow_action(struct flow_action *flow_action,
> >+ const struct tcf_exts *exts)
> >+{
> >+ const struct tc_action *act;
> >+ int num_acts = 0, i, j, k;
> >+
> >+ if (!exts)
> >+ return 0;
> >+
> >+ tcf_exts_for_each_action(i, act, exts) {
> >+ if (is_tcf_pedit(act))
> >+ num_acts += tcf_pedit_nkeys(act);
> >+ else
> >+ num_acts++;
> >+ }
> >+ if (!num_acts)
> >+ return 0;
> >+
> >+ if (flow_action_init(flow_action, num_acts) < 0)
>
> This is actually a "alloc" function. And the counterpart is "free".
I can rename it to _alloc() if you prefer.
> How about to allocate the container struct which would have the [0]
> trick for the array of action?
You mean turn *keys into keys[0] stub in struct flow_action? This is
embedded into struct tc_cls_flower_offload, I may need to make a
second look but I think it won't fly.
BTW, side note: I will rename keys to "array" given keys is not
semantically appropriate as you mentioned, BTW.
Thanks!
^ permalink raw reply
* Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
From: Jiri Pirko @ 2018-11-19 13:22 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119132141.4n4tz4mwiihwccb2@salvia>
Mon, Nov 19, 2018 at 02:21:41PM CET, pablo@netfilter.org wrote:
>On Mon, Nov 19, 2018 at 01:12:51PM +0100, Jiri Pirko wrote:
>> Mon, Nov 19, 2018 at 01:15:11AM CET, pablo@netfilter.org wrote:
>> >@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
>> > }
>> > EXPORT_SYMBOL(tc_setup_cb_call);
>> >
>> >+int tc_setup_flow_action(struct flow_action *flow_action,
>> >+ const struct tcf_exts *exts)
>> >+{
>> >+ const struct tc_action *act;
>> >+ int num_acts = 0, i, j, k;
>> >+
>> >+ if (!exts)
>> >+ return 0;
>> >+
>> >+ tcf_exts_for_each_action(i, act, exts) {
>> >+ if (is_tcf_pedit(act))
>> >+ num_acts += tcf_pedit_nkeys(act);
>> >+ else
>> >+ num_acts++;
>> >+ }
>> >+ if (!num_acts)
>> >+ return 0;
>> >+
>> >+ if (flow_action_init(flow_action, num_acts) < 0)
>>
>> This is actually a "alloc" function. And the counterpart is "free".
>
>I can rename it to _alloc() if you prefer.
>
>> How about to allocate the container struct which would have the [0]
>> trick for the array of action?
>
>You mean turn *keys into keys[0] stub in struct flow_action? This is
>embedded into struct tc_cls_flower_offload, I may need to make a
>second look but I think it won't fly.
>
>BTW, side note: I will rename keys to "array" given keys is not
>semantically appropriate as you mentioned, BTW.
What I suggest is this:
struct flow_actions {
unsinged int action_count;
struct flow_action action[0];
};
And then to have
struct flow_actions *flow_actions_alloc(unsigned int action_count)
{
return kzalloc(sizeof(struct flow_actions) + sizeof(struct flow_action) * action_count, ..);
}
Something like this.
>
>Thanks!
^ permalink raw reply
* [PATCH v3 RESEND 2/2] mm/page_alloc: use a single function to free page
From: Aaron Lu @ 2018-11-19 13:48 UTC (permalink / raw)
To: linux-mm, linux-kernel, netdev
Cc: Andrew Morton, Paweł Staszewski, Jesper Dangaard Brouer,
Eric Dumazet, Tariq Toukan, Ilias Apalodimas, Yoel Caspersen,
Mel Gorman, Saeed Mahameed, Michal Hocko, Vlastimil Babka,
Dave Hansen, Alexander Duyck, Ian Kumlien
In-Reply-To: <20181119134834.17765-1-aaron.lu@intel.com>
There are multiple places of freeing a page, they all do the same
things so a common function can be used to reduce code duplicate.
It also avoids bug fixed in one function but left in another.
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
mm/page_alloc.c | 37 ++++++++++++++-----------------------
1 file changed, 14 insertions(+), 23 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8f8c6b33b637..93cc8e686eca 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4547,16 +4547,19 @@ unsigned long get_zeroed_page(gfp_t gfp_mask)
}
EXPORT_SYMBOL(get_zeroed_page);
-void __free_pages(struct page *page, unsigned int order)
+static inline void free_the_page(struct page *page, unsigned int order)
{
- if (put_page_testzero(page)) {
- if (order == 0)
- free_unref_page(page);
- else
- __free_pages_ok(page, order);
- }
+ if (order == 0)
+ free_unref_page(page);
+ else
+ __free_pages_ok(page, order);
}
+void __free_pages(struct page *page, unsigned int order)
+{
+ if (put_page_testzero(page))
+ free_the_page(page, order);
+}
EXPORT_SYMBOL(__free_pages);
void free_pages(unsigned long addr, unsigned int order)
@@ -4605,14 +4608,8 @@ void __page_frag_cache_drain(struct page *page, unsigned int count)
{
VM_BUG_ON_PAGE(page_ref_count(page) == 0, page);
- if (page_ref_sub_and_test(page, count)) {
- unsigned int order = compound_order(page);
-
- if (order == 0)
- free_unref_page(page);
- else
- __free_pages_ok(page, order);
- }
+ if (page_ref_sub_and_test(page, count))
+ free_the_page(page, compound_order(page));
}
EXPORT_SYMBOL(__page_frag_cache_drain);
@@ -4677,14 +4674,8 @@ void page_frag_free(void *addr)
{
struct page *page = virt_to_head_page(addr);
- if (unlikely(put_page_testzero(page))) {
- unsigned int order = compound_order(page);
-
- if (order == 0)
- free_unref_page(page);
- else
- __free_pages_ok(page, order);
- }
+ if (unlikely(put_page_testzero(page)))
+ free_the_page(page, compound_order(page));
}
EXPORT_SYMBOL(page_frag_free);
--
2.17.2
^ permalink raw reply related
* Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
From: Jiri Pirko @ 2018-11-19 13:57 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-6-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:12AM CET, pablo@netfilter.org wrote:
>This patch provides a tc_cls_flower_stats structure that acts as
>container for tc_cls_flower_offload, then we can use to restore the
>statistics on the existing TC actions. Hence, tcf_exts_stats_update() is
>not used from drivers.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 4 ++--
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 6 +++---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +-
> drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +-
> drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 +++---
> include/net/pkt_cls.h | 15 +++++++++++++++
> net/sched/cls_flower.c | 4 ++++
> 7 files changed, 29 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>index b82143d6cdde..3d71b2530d67 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>@@ -1366,8 +1366,8 @@ static int bnxt_tc_get_flow_stats(struct bnxt *bp,
> lastused = flow->lastused;
> spin_unlock(&flow->stats_lock);
>
>- tcf_exts_stats_update(tc_flow_cmd->exts, stats.bytes, stats.packets,
>- lastused);
>+ tc_cls_flower_stats_update(tc_flow_cmd, stats.bytes, stats.packets,
>+ lastused);
> return 0;
> }
>
>diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>index 39c5af5dad3d..2c7d1aebe214 100644
>--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>@@ -807,9 +807,9 @@ int cxgb4_tc_flower_stats(struct net_device *dev,
> if (ofld_stats->packet_count != packets) {
> if (ofld_stats->prev_packet_count != packets)
> ofld_stats->last_used = jiffies;
>- tcf_exts_stats_update(cls->exts, bytes - ofld_stats->byte_count,
>- packets - ofld_stats->packet_count,
>- ofld_stats->last_used);
>+ tc_cls_flower_stats_update(cls, bytes - ofld_stats->byte_count,
>+ packets - ofld_stats->packet_count,
>+ ofld_stats->last_used);
>
> ofld_stats->packet_count = packets;
> ofld_stats->byte_count = bytes;
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>index 2645e5d1e790..c5f0b826fa91 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>@@ -3224,7 +3224,7 @@ int mlx5e_stats_flower(struct mlx5e_priv *priv,
>
> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
>
>- tcf_exts_stats_update(f->exts, bytes, packets, lastuse);
>+ tc_cls_flower_stats_update(f, bytes, packets, lastuse);
>
> return 0;
> }
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
>index 193a6f9acf79..3398984ffb2a 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
>@@ -460,7 +460,7 @@ int mlxsw_sp_flower_stats(struct mlxsw_sp *mlxsw_sp,
> if (err)
> goto err_rule_get_stats;
>
>- tcf_exts_stats_update(f->exts, bytes, packets, lastuse);
>+ tc_cls_flower_stats_update(f, bytes, packets, lastuse);
>
> mlxsw_sp_acl_ruleset_put(mlxsw_sp, ruleset);
> return 0;
>diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>index 708331234908..bec74d84756c 100644
>--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
>+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
>@@ -532,9 +532,9 @@ nfp_flower_get_stats(struct nfp_app *app, struct net_device *netdev,
> ctx_id = be32_to_cpu(nfp_flow->meta.host_ctx_id);
>
> spin_lock_bh(&priv->stats_lock);
>- tcf_exts_stats_update(flow->exts, priv->stats[ctx_id].bytes,
>- priv->stats[ctx_id].pkts,
>- priv->stats[ctx_id].used);
>+ tc_cls_flower_stats_update(flow, priv->stats[ctx_id].bytes,
>+ priv->stats[ctx_id].pkts,
>+ priv->stats[ctx_id].used);
>
> priv->stats[ctx_id].pkts = 0;
> priv->stats[ctx_id].bytes = 0;
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 7d7aefa5fcd2..7f9a8d5ca945 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -758,6 +758,12 @@ enum tc_fl_command {
> TC_CLSFLOWER_TMPLT_DESTROY,
> };
>
>+struct tc_cls_flower_stats {
>+ u64 pkts;
>+ u64 bytes;
>+ u64 lastused;
>+};
>+
> struct tc_cls_flower_offload {
> struct tc_cls_common_offload common;
> enum tc_fl_command command;
>@@ -765,6 +771,7 @@ struct tc_cls_flower_offload {
> struct flow_rule rule;
> struct tcf_exts *exts;
> u32 classid;
>+ struct tc_cls_flower_stats stats;
> };
>
> static inline struct flow_rule *
>@@ -773,6 +780,14 @@ tc_cls_flower_offload_flow_rule(struct tc_cls_flower_offload *tc_flow_cmd)
> return &tc_flow_cmd->rule;
> }
>
>+static inline void tc_cls_flower_stats_update(struct tc_cls_flower_offload *cls_flower,
>+ u64 pkts, u64 bytes, u64 lastused)
>+{
>+ cls_flower->stats.pkts = pkts;
>+ cls_flower->stats.bytes = bytes;
>+ cls_flower->stats.lastused = lastused;
Why do you need to store the values here in struct tc_cls_flower_offload?
Why don't you just call tcf_exts_stats_update()? Basically,
tc_cls_flower_stats_update() should be just wrapper around
tcf_exts_stats_update() so that drivers wouldn't use ->exts directly, as
you will remove them in follow-up patches, no?
>+}
>+
> enum tc_matchall_command {
> TC_CLSMATCHALL_REPLACE,
> TC_CLSMATCHALL_DESTROY,
>diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>index a301fb8e68e7..ee67f1ae8786 100644
>--- a/net/sched/cls_flower.c
>+++ b/net/sched/cls_flower.c
>@@ -430,6 +430,10 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
>
> tc_setup_cb_call(block, &f->exts, TC_SETUP_CLSFLOWER,
> &cls_flower, false);
>+
>+ tcf_exts_stats_update(&f->exts, cls_flower.stats.bytes,
>+ cls_flower.stats.pkts,
>+ cls_flower.stats.lastused);
> }
>
> static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
>--
>2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next,v2 08/12] flow_dissector: add wake-up-on-lan and queue to flow_action
From: Jiri Pirko @ 2018-11-19 13:59 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-9-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:15AM CET, pablo@netfilter.org wrote:
>These actions need to be added to support bcm sf2 features available
>through the ethtool_rx_flow interface.
>
>Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
For me it would be nicer to have this as 2 patches, one per action type.
But up to you.
>---
>v2: no changes.
>
> include/net/flow_dissector.h | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 925c208816f1..7a4683646d5a 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -418,6 +418,8 @@ enum flow_action_key_id {
> FLOW_ACTION_KEY_ADD,
> FLOW_ACTION_KEY_CSUM,
> FLOW_ACTION_KEY_MARK,
>+ FLOW_ACTION_KEY_WAKE,
>+ FLOW_ACTION_KEY_QUEUE,
> };
>
> /* This is mirroring enum pedit_header_type definition for easy mapping between
>@@ -452,6 +454,7 @@ struct flow_action_key {
> const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
> u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
> u32 mark; /* FLOW_ACTION_KEY_MARK */
>+ u32 queue_index; /* FLOW_ACTION_KEY_QUEUE */
> };
> };
>
>--
>2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next] MAINTAINERS: Add myself as third phylib maintainer
From: Andrew Lunn @ 2018-11-19 14:09 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <acdab6fa-b552-5358-6095-8ebe6a46c99b@gmail.com>
On Mon, Nov 19, 2018 at 08:01:03AM +0100, Heiner Kallweit wrote:
> Add myself as third phylib maintainer.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure
From: Jiri Pirko @ 2018-11-19 14:03 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-4-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:10AM CET, pablo@netfilter.org wrote:
>This new infrastructure defines the nic actions that you can perform
>from existing network drivers. This infrastructure allows us to avoid a
>direct dependency with the native software TC action representation.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> include/net/flow_dissector.h | 70 ++++++++++++++++++++++++++++++++++++++++++++
> net/core/flow_dissector.c | 18 ++++++++++++
> 2 files changed, 88 insertions(+)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 965a82b8d881..925c208816f1 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -402,8 +402,78 @@ void flow_rule_match_enc_keyid(const struct flow_rule *rule,
> void flow_rule_match_enc_opts(const struct flow_rule *rule,
> struct flow_match_enc_opts *out);
>
>+enum flow_action_key_id {
>+ FLOW_ACTION_KEY_ACCEPT = 0,
>+ FLOW_ACTION_KEY_DROP,
>+ FLOW_ACTION_KEY_TRAP,
>+ FLOW_ACTION_KEY_GOTO,
>+ FLOW_ACTION_KEY_REDIRECT,
>+ FLOW_ACTION_KEY_MIRRED,
>+ FLOW_ACTION_KEY_VLAN_PUSH,
>+ FLOW_ACTION_KEY_VLAN_POP,
>+ FLOW_ACTION_KEY_VLAN_MANGLE,
>+ FLOW_ACTION_KEY_TUNNEL_ENCAP,
>+ FLOW_ACTION_KEY_TUNNEL_DECAP,
>+ FLOW_ACTION_KEY_MANGLE,
>+ FLOW_ACTION_KEY_ADD,
>+ FLOW_ACTION_KEY_CSUM,
>+ FLOW_ACTION_KEY_MARK,
>+};
>+
>+/* This is mirroring enum pedit_header_type definition for easy mapping between
>+ * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
>+ * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
>+ */
>+enum flow_act_mangle_base {
>+ FLOW_ACT_MANGLE_UNSPEC = 0,
>+ FLOW_ACT_MANGLE_HDR_TYPE_ETH,
>+ FLOW_ACT_MANGLE_HDR_TYPE_IP4,
>+ FLOW_ACT_MANGLE_HDR_TYPE_IP6,
>+ FLOW_ACT_MANGLE_HDR_TYPE_TCP,
>+ FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>+};
>+
>+struct flow_action_key {
>+ enum flow_action_key_id id;
>+ union {
>+ u32 chain_index; /* FLOW_ACTION_KEY_GOTO */
>+ struct net_device *dev; /* FLOW_ACTION_KEY_REDIRECT */
>+ struct { /* FLOW_ACTION_KEY_VLAN */
>+ u16 vid;
>+ __be16 proto;
>+ u8 prio;
>+ } vlan;
>+ struct { /* FLOW_ACTION_KEY_PACKET_EDIT */
>+ enum flow_act_mangle_base htype;
>+ u32 offset;
>+ u32 mask;
>+ u32 val;
>+ } mangle;
>+ const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
>+ u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
>+ u32 mark; /* FLOW_ACTION_KEY_MARK */
>+ };
>+};
>+
>+struct flow_action {
Hmm, thinking about it a bit more, none of this is is related to flow
dissector so it is misleading to put the code in flow_dissector.[hc].
Maybe you can push this and related stuff into new files include/net/flow.h
and net/core/flow.c.
>+ int num_keys;
>+ struct flow_action_key *keys;
>+};
>+
>+int flow_action_init(struct flow_action *flow_action, int num_acts);
>+void flow_action_free(struct flow_action *flow_action);
>+
>+static inline bool flow_action_has_keys(const struct flow_action *action)
>+{
>+ return action->num_keys;
>+}
>+
>+#define flow_action_for_each(__i, __act, __actions) \
>+ for (__i = 0, __act = &(__actions)->keys[0]; __i < (__actions)->num_keys; __act = &(__actions)->keys[++__i])
>+
> struct flow_rule {
> struct flow_match match;
>+ struct flow_action action;
> };
>
> static inline bool flow_rule_match_key(const struct flow_rule *rule,
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 186089b8d852..b9368349f0f7 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -258,6 +258,24 @@ void flow_rule_match_enc_opts(const struct flow_rule *rule,
> }
> EXPORT_SYMBOL(flow_rule_match_enc_opts);
>
>+int flow_action_init(struct flow_action *flow_action, int num_acts)
>+{
>+ flow_action->keys = kmalloc(sizeof(struct flow_action_key) * num_acts,
>+ GFP_KERNEL);
>+ if (!flow_action->keys)
>+ return -ENOMEM;
>+
>+ flow_action->num_keys = num_acts;
>+ return 0;
>+}
>+EXPORT_SYMBOL(flow_action_init);
>+
>+void flow_action_free(struct flow_action *flow_action)
>+{
>+ kfree(flow_action->keys);
>+}
>+EXPORT_SYMBOL(flow_action_free);
>+
> /**
> * __skb_flow_get_ports - extract the upper layer ports and return them
> * @skb: sk_buff to extract the ports from
>--
>2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next,v2 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
From: Jiri Pirko @ 2018-11-19 14:17 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-10-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:16AM CET, pablo@netfilter.org wrote:
>This patch adds a function to translate the ethtool_rx_flow_spec
>structure to the flow_rule representation.
>
>This allows us to reuse code from the driver side given that both flower
>and ethtool_rx_flow interfaces use the same representation.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> include/net/flow_dissector.h | 5 ++
> net/core/flow_dissector.c | 190 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 195 insertions(+)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 7a4683646d5a..ec9036232538 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -485,4 +485,9 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
> return dissector_uses_key(rule->match.dissector, key);
> }
>
>+struct ethtool_rx_flow_spec;
>+
>+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs);
>+void ethtool_rx_flow_rule_free(struct flow_rule *rule);
>+
> #endif
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index b9368349f0f7..ef5bdb62620c 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -17,6 +17,7 @@
> #include <linux/dccp.h>
> #include <linux/if_tunnel.h>
> #include <linux/if_pppox.h>
>+#include <uapi/linux/ethtool.h>
> #include <linux/ppp_defs.h>
> #include <linux/stddef.h>
> #include <linux/if_ether.h>
>@@ -276,6 +277,195 @@ void flow_action_free(struct flow_action *flow_action)
> }
> EXPORT_SYMBOL(flow_action_free);
>
>+struct ethtool_rx_flow_key {
>+ struct flow_dissector_key_basic basic;
>+ union {
>+ struct flow_dissector_key_ipv4_addrs ipv4;
>+ struct flow_dissector_key_ipv6_addrs ipv6;
>+ };
>+ struct flow_dissector_key_ports tp;
>+ struct flow_dissector_key_ip ip;
>+} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
>+
>+struct ethtool_rx_flow_match {
>+ struct flow_dissector dissector;
>+ struct ethtool_rx_flow_key key;
>+ struct ethtool_rx_flow_key mask;
>+};
>+
>+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs)
>+{
>+ static struct in6_addr zero_addr = {};
>+ struct ethtool_rx_flow_match *match;
>+ struct flow_action_key *act;
>+ struct flow_rule *rule;
>+
>+ rule = kmalloc(sizeof(struct flow_rule), GFP_KERNEL);
>+ if (!rule)
>+ return NULL;
>+
>+ match = kzalloc(sizeof(struct ethtool_rx_flow_match), GFP_KERNEL);
>+ if (!match)
>+ goto err_match;
>+
>+ rule->match.dissector = &match->dissector;
>+ rule->match.mask = &match->mask;
>+ rule->match.key = &match->key;
>+
>+ match->mask.basic.n_proto = 0xffff;
>+
>+ switch (fs->flow_type & ~FLOW_EXT) {
>+ case TCP_V4_FLOW:
>+ case UDP_V4_FLOW: {
>+ const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec;
>+
>+ match->key.basic.n_proto = htons(ETH_P_IP);
>+
>+ v4_spec = &fs->h_u.tcp_ip4_spec;
>+ v4_m_spec = &fs->m_u.tcp_ip4_spec;
>+
>+ if (v4_m_spec->ip4src) {
>+ match->key.ipv4.src = v4_spec->ip4src;
>+ match->mask.ipv4.src = v4_m_spec->ip4src;
>+ }
>+ if (v4_m_spec->ip4dst) {
>+ match->key.ipv4.dst = v4_spec->ip4dst;
>+ match->mask.ipv4.dst = v4_m_spec->ip4dst;
>+ }
>+ if (v4_m_spec->ip4src ||
>+ v4_m_spec->ip4dst) {
>+ match->dissector.used_keys |=
>+ FLOW_DISSECTOR_KEY_IPV4_ADDRS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IPV4_ADDRS] =
>+ offsetof(struct ethtool_rx_flow_key, ipv4);
>+ }
>+ if (v4_m_spec->psrc) {
>+ match->key.tp.src = v4_spec->psrc;
>+ match->mask.tp.src = v4_m_spec->psrc;
>+ }
>+ if (v4_m_spec->pdst) {
>+ match->key.tp.dst = v4_spec->pdst;
>+ match->mask.tp.dst = v4_m_spec->pdst;
>+ }
>+ if (v4_m_spec->psrc ||
>+ v4_m_spec->pdst) {
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
>+ offsetof(struct ethtool_rx_flow_key, tp);
>+ }
>+ if (v4_m_spec->tos) {
>+ match->key.ip.tos = v4_spec->pdst;
>+ match->mask.ip.tos = v4_m_spec->pdst;
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
>+ offsetof(struct ethtool_rx_flow_key, ip);
>+ }
>+ }
>+ break;
>+ case TCP_V6_FLOW:
>+ case UDP_V6_FLOW: {
>+ const struct ethtool_tcpip6_spec *v6_spec, *v6_m_spec;
>+
>+ match->key.basic.n_proto = htons(ETH_P_IPV6);
>+
>+ v6_spec = &fs->h_u.tcp_ip6_spec;
>+ v6_m_spec = &fs->m_u.tcp_ip6_spec;
>+ if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
>+ memcpy(&match->key.ipv6.src, v6_spec->ip6src,
>+ sizeof(match->key.ipv6.src));
>+ memcpy(&match->mask.ipv6.src, v6_m_spec->ip6src,
>+ sizeof(match->mask.ipv6.src));
>+ }
>+ if (memcmp(v6_m_spec->ip6dst, &zero_addr, sizeof(zero_addr))) {
>+ memcpy(&match->key.ipv6.dst, v6_spec->ip6dst,
>+ sizeof(match->key.ipv6.dst));
>+ memcpy(&match->mask.ipv6.dst, v6_m_spec->ip6dst,
>+ sizeof(match->mask.ipv6.dst));
>+ }
>+ if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr)) ||
>+ memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
>+ match->dissector.used_keys |=
>+ FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IPV6_ADDRS] =
>+ offsetof(struct ethtool_rx_flow_key, ipv6);
>+ }
>+ if (v6_m_spec->psrc) {
>+ match->key.tp.src = v6_spec->psrc;
>+ match->mask.tp.src = v6_m_spec->psrc;
>+ }
>+ if (v6_m_spec->pdst) {
>+ match->key.tp.dst = v6_spec->pdst;
>+ match->mask.tp.dst = v6_m_spec->pdst;
>+ }
>+ if (v6_m_spec->psrc ||
>+ v6_m_spec->pdst) {
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
>+ offsetof(struct ethtool_rx_flow_key, tp);
>+ }
>+ if (v6_m_spec->tclass) {
>+ match->key.ip.tos = v6_spec->tclass;
>+ match->mask.ip.tos = v6_m_spec->tclass;
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
>+ offsetof(struct ethtool_rx_flow_key, ip);
>+ }
>+ }
>+ break;
>+ }
>+
>+ switch (fs->flow_type & ~FLOW_EXT) {
>+ case TCP_V4_FLOW:
>+ case TCP_V6_FLOW:
>+ match->key.basic.ip_proto = IPPROTO_TCP;
>+ break;
>+ case UDP_V4_FLOW:
>+ case UDP_V6_FLOW:
>+ match->key.basic.ip_proto = IPPROTO_UDP;
>+ break;
>+ }
>+ match->mask.basic.ip_proto = 0xff;
>+
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_BASIC;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_BASIC] =
>+ offsetof(struct ethtool_rx_flow_key, basic);
>+
>+ /* ethtool_rx supports only one single action per rule. */
>+ if (flow_action_init(&rule->action, 1) < 0)
>+ goto err_action;
>+
>+ act = &rule->action.keys[0];
>+ switch (fs->ring_cookie) {
>+ case RX_CLS_FLOW_DISC:
>+ act->id = FLOW_ACTION_KEY_DROP;
>+ break;
>+ case RX_CLS_FLOW_WAKE:
>+ act->id = FLOW_ACTION_KEY_WAKE;
>+ break;
>+ default:
>+ act->id = FLOW_ACTION_KEY_QUEUE;
>+ act->queue_index = fs->ring_cookie;
>+ break;
>+ }
>+
>+ return rule;
>+
>+err_action:
>+ kfree(match);
>+err_match:
>+ kfree(rule);
>+ return NULL;
>+}
>+EXPORT_SYMBOL(ethtool_rx_flow_rule);
>+
>+void ethtool_rx_flow_rule_free(struct flow_rule *rule)
>+{
>+ kfree((struct flow_match *)rule->match.dissector);
>+ flow_action_free(&rule->action);
>+ kfree(rule);
>+}
>+EXPORT_SYMBOL(ethtool_rx_flow_rule_free);
Code that is doing ethtool->flow_rule conversion should be in ethtool
code. You should just provide helpers here (in flow.c) that could be
used to assemble the flow_rule structure.
Same applies to TC.
>+
> /**
> * __skb_flow_get_ports - extract the upper layer ports and return them
> * @skb: sk_buff to extract the ports from
>--
>2.11.0
>
^ permalink raw reply
* [PATCH net 0/2] qed: Fix Queue Manager getters
From: Denis Bolotin @ 2018-11-19 14:28 UTC (permalink / raw)
To: davem, netdev; +Cc: ariel.elior, Denis Bolotin
Hi Dave,
This patch series fixes various queue manager getter functions. It is
important to make sure the getter's caller will receive a valid queue even
in error case to prevent more serious bugs.
Please consider applying to net.
Thanks,
Denis
Denis Bolotin (2):
qed: Fix bitmap_weight() check
qed: Fix QM getters to always return a valid pq
drivers/net/ethernet/qlogic/qed/qed_dev.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH net 1/2] qed: Fix bitmap_weight() check
From: Denis Bolotin @ 2018-11-19 14:28 UTC (permalink / raw)
To: davem, netdev; +Cc: ariel.elior, Denis Bolotin, Michal Kalderon
In-Reply-To: <20181119142831.8035-1-denis.bolotin@cavium.com>
Fix the condition which verifies that only one flag is set. The API
bitmap_weight() should receive size in bits instead of bytes.
Fixes: b5a9ee7cf3be ("qed: Revise QM cofiguration")
Signed-off-by: Denis Bolotin <denis.bolotin@cavium.com>
Signed-off-by: Michal Kalderon <michal.kalderon@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index cff1410..9b41e4b 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -485,8 +485,11 @@ static u16 *qed_init_qm_get_idx_from_flags(struct qed_hwfn *p_hwfn,
struct qed_qm_info *qm_info = &p_hwfn->qm_info;
/* Can't have multiple flags set here */
- if (bitmap_weight((unsigned long *)&pq_flags, sizeof(pq_flags)) > 1)
+ if (bitmap_weight((unsigned long *)&pq_flags,
+ sizeof(pq_flags) * BITS_PER_BYTE) > 1) {
+ DP_ERR(p_hwfn, "requested multiple pq flags 0x%x\n", pq_flags);
goto err;
+ }
switch (pq_flags) {
case PQ_FLAGS_RLS:
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 2/2] qed: Fix QM getters to always return a valid pq
From: Denis Bolotin @ 2018-11-19 14:28 UTC (permalink / raw)
To: davem, netdev; +Cc: ariel.elior, Denis Bolotin, Michal Kalderon
In-Reply-To: <20181119142831.8035-1-denis.bolotin@cavium.com>
The getter callers doesn't know the valid Physical Queues (PQ) values.
This patch makes sure that a valid PQ will always be returned.
The patch consists of 3 fixes:
- When qed_init_qm_get_idx_from_flags() receives a disabled flag, it
returned PQ 0, which can potentially be another function's pq. Verify
that flag is enabled, otherwise return default start_pq.
- When qed_init_qm_get_idx_from_flags() receives an unknown flag, it
returned NULL and could lead to a segmentation fault. Return default
start_pq instead.
- A modulo operation was added to MCOS/VFS PQ getters to make sure the
PQ returned is in range of the required flag.
Fixes: b5a9ee7cf3be ("qed: Revise QM cofiguration")
Signed-off-by: Denis Bolotin <denis.bolotin@cavium.com>
Signed-off-by: Michal Kalderon <michal.kalderon@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 9b41e4b..88a8576 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -491,6 +491,11 @@ static u16 *qed_init_qm_get_idx_from_flags(struct qed_hwfn *p_hwfn,
goto err;
}
+ if (!(qed_get_pq_flags(p_hwfn) & pq_flags)) {
+ DP_ERR(p_hwfn, "pq flag 0x%x is not set\n", pq_flags);
+ goto err;
+ }
+
switch (pq_flags) {
case PQ_FLAGS_RLS:
return &qm_info->first_rl_pq;
@@ -513,8 +518,7 @@ static u16 *qed_init_qm_get_idx_from_flags(struct qed_hwfn *p_hwfn,
}
err:
- DP_ERR(p_hwfn, "BAD pq flags %d\n", pq_flags);
- return NULL;
+ return &qm_info->start_pq;
}
/* save pq index in qm info */
@@ -538,20 +542,32 @@ u16 qed_get_cm_pq_idx_mcos(struct qed_hwfn *p_hwfn, u8 tc)
{
u8 max_tc = qed_init_qm_get_num_tcs(p_hwfn);
+ if (max_tc == 0) {
+ DP_ERR(p_hwfn, "pq with flag 0x%lx do not exist\n",
+ PQ_FLAGS_MCOS);
+ return p_hwfn->qm_info.start_pq;
+ }
+
if (tc > max_tc)
DP_ERR(p_hwfn, "tc %d must be smaller than %d\n", tc, max_tc);
- return qed_get_cm_pq_idx(p_hwfn, PQ_FLAGS_MCOS) + tc;
+ return qed_get_cm_pq_idx(p_hwfn, PQ_FLAGS_MCOS) + (tc % max_tc);
}
u16 qed_get_cm_pq_idx_vf(struct qed_hwfn *p_hwfn, u16 vf)
{
u16 max_vf = qed_init_qm_get_num_vfs(p_hwfn);
+ if (max_vf == 0) {
+ DP_ERR(p_hwfn, "pq with flag 0x%lx do not exist\n",
+ PQ_FLAGS_VFS);
+ return p_hwfn->qm_info.start_pq;
+ }
+
if (vf > max_vf)
DP_ERR(p_hwfn, "vf %d must be smaller than %d\n", vf, max_vf);
- return qed_get_cm_pq_idx(p_hwfn, PQ_FLAGS_VFS) + vf;
+ return qed_get_cm_pq_idx(p_hwfn, PQ_FLAGS_VFS) + (vf % max_vf);
}
u16 qed_get_cm_pq_idx_ofld_mtc(struct qed_hwfn *p_hwfn, u8 tc)
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface
From: Lorenz Bauer @ 2018-11-19 14:30 UTC (permalink / raw)
To: ys114321; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAH3MdRW1=iUm0qQdNrv9r5Orn3rZ2XBDn-qym5i_cpi7wo6PxA@mail.gmail.com>
On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.com> wrote:
>
> There is a slight change of user space behavior for this patch.
> Without this patch, the value bpf_attr.test.data_size_out is output only.
> For example,
> output buffer : out_buf (user allocated size 10)
> data_size_out is a random value (e.g., 1),
>
> The actual data to copy is 5.
>
> In today's implementation, the kernel will copy 5 and set data_size_out is 5.
>
> With this patch, the kernel will copy 1 and set data_size_out is 5.
>
> I am not 100% sure at this time whether we CAN overload data_size_out
> since it MAY break existing applications.
Yes, that's correct. I think that the likelihood of this is low. It would
affect code that uses bpf_attr without zeroing it first. I had a look around,
and I could only find code that would keep working:
* kernel libbpf and descendants (e.g katran)
* github.com/iovisor/gobpf
* github.com/newtools/ebpf
That doesn't really guarantee anything of course.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure
From: Pablo Neira Ayuso @ 2018-11-19 14:42 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119140322.GI2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 03:03:22PM +0100, Jiri Pirko wrote:
[...]
> >+struct flow_action {
>
> Hmm, thinking about it a bit more, none of this is is related to flow
> dissector so it is misleading to put the code in flow_dissector.[hc].
>
> Maybe you can push this and related stuff into new files include/net/flow.h
> and net/core/flow.c.
Will do this, thanks Jiri.
^ permalink raw reply
* Re: [PATCH net-next,v2 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
From: Pablo Neira Ayuso @ 2018-11-19 14:43 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119141743.GJ2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 03:17:43PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:16AM CET, pablo@netfilter.org wrote:
[...]
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index 7a4683646d5a..ec9036232538 100644
> >--- a/include/net/flow_dissector.h
> >+++ b/include/net/flow_dissector.h
[...]
> >+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs)
[...]
> Code that is doing ethtool->flow_rule conversion should be in ethtool
> code. You should just provide helpers here (in flow.c) that could be
> used to assemble the flow_rule structure.
OK, will place this in ethtool file.
^ permalink raw reply
* Re: [PATCH net-next,v2 05/12] cls_flower: add statistics retrieval infrastructure and use it
From: Pablo Neira Ayuso @ 2018-11-19 14:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119135705.GG2223@nanopsycho.orion>
On Mon, Nov 19, 2018 at 02:57:05PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:12AM CET, pablo@netfilter.org wrote:
[...]
> >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> >index 7d7aefa5fcd2..7f9a8d5ca945 100644
> >--- a/include/net/pkt_cls.h
> >+++ b/include/net/pkt_cls.h
> >@@ -758,6 +758,12 @@ enum tc_fl_command {
> > TC_CLSFLOWER_TMPLT_DESTROY,
> > };
> >
> >+struct tc_cls_flower_stats {
> >+ u64 pkts;
> >+ u64 bytes;
> >+ u64 lastused;
> >+};
> >+
> > struct tc_cls_flower_offload {
> > struct tc_cls_common_offload common;
> > enum tc_fl_command command;
> >@@ -765,6 +771,7 @@ struct tc_cls_flower_offload {
> > struct flow_rule rule;
> > struct tcf_exts *exts;
> > u32 classid;
> >+ struct tc_cls_flower_stats stats;
> > };
> >
> > static inline struct flow_rule *
> >@@ -773,6 +780,14 @@ tc_cls_flower_offload_flow_rule(struct tc_cls_flower_offload *tc_flow_cmd)
> > return &tc_flow_cmd->rule;
> > }
> >
> >+static inline void tc_cls_flower_stats_update(struct tc_cls_flower_offload *cls_flower,
> >+ u64 pkts, u64 bytes, u64 lastused)
> >+{
> >+ cls_flower->stats.pkts = pkts;
> >+ cls_flower->stats.bytes = bytes;
> >+ cls_flower->stats.lastused = lastused;
>
> Why do you need to store the values here in struct tc_cls_flower_offload?
> Why don't you just call tcf_exts_stats_update()? Basically,
> tc_cls_flower_stats_update() should be just wrapper around
> tcf_exts_stats_update() so that drivers wouldn't use ->exts directly, as
> you will remove them in follow-up patches, no?
Patch 07/12 stops exposing tc action exts to drivers, so we need a
structure (struct tc_cls_flower_stats) to convey this statistics back
to the cls_flower frontend.
^ permalink raw reply
* Re: [PATCH net-next,v2 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
From: Jiri Pirko @ 2018-11-19 14:49 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz
In-Reply-To: <20181119001519.12124-10-pablo@netfilter.org>
Mon, Nov 19, 2018 at 01:15:16AM CET, pablo@netfilter.org wrote:
>This patch adds a function to translate the ethtool_rx_flow_spec
>structure to the flow_rule representation.
>
>This allows us to reuse code from the driver side given that both flower
>and ethtool_rx_flow interfaces use the same representation.
>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
>v2: no changes.
>
> include/net/flow_dissector.h | 5 ++
> net/core/flow_dissector.c | 190 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 195 insertions(+)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 7a4683646d5a..ec9036232538 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -485,4 +485,9 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
> return dissector_uses_key(rule->match.dissector, key);
> }
>
>+struct ethtool_rx_flow_spec;
>+
>+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs);
>+void ethtool_rx_flow_rule_free(struct flow_rule *rule);
>+
> #endif
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index b9368349f0f7..ef5bdb62620c 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -17,6 +17,7 @@
> #include <linux/dccp.h>
> #include <linux/if_tunnel.h>
> #include <linux/if_pppox.h>
>+#include <uapi/linux/ethtool.h>
> #include <linux/ppp_defs.h>
> #include <linux/stddef.h>
> #include <linux/if_ether.h>
>@@ -276,6 +277,195 @@ void flow_action_free(struct flow_action *flow_action)
> }
> EXPORT_SYMBOL(flow_action_free);
>
>+struct ethtool_rx_flow_key {
>+ struct flow_dissector_key_basic basic;
>+ union {
>+ struct flow_dissector_key_ipv4_addrs ipv4;
>+ struct flow_dissector_key_ipv6_addrs ipv6;
>+ };
>+ struct flow_dissector_key_ports tp;
>+ struct flow_dissector_key_ip ip;
>+} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
>+
>+struct ethtool_rx_flow_match {
>+ struct flow_dissector dissector;
>+ struct ethtool_rx_flow_key key;
>+ struct ethtool_rx_flow_key mask;
>+};
>+
>+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs)
>+{
>+ static struct in6_addr zero_addr = {};
>+ struct ethtool_rx_flow_match *match;
>+ struct flow_action_key *act;
>+ struct flow_rule *rule;
>+
>+ rule = kmalloc(sizeof(struct flow_rule), GFP_KERNEL);
Allocating struct flow_rule manually here seems wrong. There should
be some helpers. Please see below.***
>+ if (!rule)
>+ return NULL;
>+
>+ match = kzalloc(sizeof(struct ethtool_rx_flow_match), GFP_KERNEL);
>+ if (!match)
>+ goto err_match;
>+
>+ rule->match.dissector = &match->dissector;
>+ rule->match.mask = &match->mask;
>+ rule->match.key = &match->key;
>+
>+ match->mask.basic.n_proto = 0xffff;
>+
>+ switch (fs->flow_type & ~FLOW_EXT) {
>+ case TCP_V4_FLOW:
>+ case UDP_V4_FLOW: {
>+ const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec;
>+
>+ match->key.basic.n_proto = htons(ETH_P_IP);
>+
>+ v4_spec = &fs->h_u.tcp_ip4_spec;
>+ v4_m_spec = &fs->m_u.tcp_ip4_spec;
>+
>+ if (v4_m_spec->ip4src) {
>+ match->key.ipv4.src = v4_spec->ip4src;
>+ match->mask.ipv4.src = v4_m_spec->ip4src;
>+ }
>+ if (v4_m_spec->ip4dst) {
>+ match->key.ipv4.dst = v4_spec->ip4dst;
>+ match->mask.ipv4.dst = v4_m_spec->ip4dst;
>+ }
>+ if (v4_m_spec->ip4src ||
>+ v4_m_spec->ip4dst) {
>+ match->dissector.used_keys |=
>+ FLOW_DISSECTOR_KEY_IPV4_ADDRS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IPV4_ADDRS] =
>+ offsetof(struct ethtool_rx_flow_key, ipv4);
>+ }
>+ if (v4_m_spec->psrc) {
>+ match->key.tp.src = v4_spec->psrc;
>+ match->mask.tp.src = v4_m_spec->psrc;
>+ }
>+ if (v4_m_spec->pdst) {
>+ match->key.tp.dst = v4_spec->pdst;
>+ match->mask.tp.dst = v4_m_spec->pdst;
>+ }
>+ if (v4_m_spec->psrc ||
>+ v4_m_spec->pdst) {
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
>+ offsetof(struct ethtool_rx_flow_key, tp);
>+ }
>+ if (v4_m_spec->tos) {
>+ match->key.ip.tos = v4_spec->pdst;
>+ match->mask.ip.tos = v4_m_spec->pdst;
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
>+ offsetof(struct ethtool_rx_flow_key, ip);
>+ }
>+ }
>+ break;
>+ case TCP_V6_FLOW:
>+ case UDP_V6_FLOW: {
>+ const struct ethtool_tcpip6_spec *v6_spec, *v6_m_spec;
>+
>+ match->key.basic.n_proto = htons(ETH_P_IPV6);
>+
>+ v6_spec = &fs->h_u.tcp_ip6_spec;
>+ v6_m_spec = &fs->m_u.tcp_ip6_spec;
>+ if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
>+ memcpy(&match->key.ipv6.src, v6_spec->ip6src,
>+ sizeof(match->key.ipv6.src));
>+ memcpy(&match->mask.ipv6.src, v6_m_spec->ip6src,
>+ sizeof(match->mask.ipv6.src));
>+ }
>+ if (memcmp(v6_m_spec->ip6dst, &zero_addr, sizeof(zero_addr))) {
>+ memcpy(&match->key.ipv6.dst, v6_spec->ip6dst,
>+ sizeof(match->key.ipv6.dst));
>+ memcpy(&match->mask.ipv6.dst, v6_m_spec->ip6dst,
>+ sizeof(match->mask.ipv6.dst));
>+ }
>+ if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr)) ||
>+ memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
>+ match->dissector.used_keys |=
>+ FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IPV6_ADDRS] =
>+ offsetof(struct ethtool_rx_flow_key, ipv6);
>+ }
>+ if (v6_m_spec->psrc) {
>+ match->key.tp.src = v6_spec->psrc;
>+ match->mask.tp.src = v6_m_spec->psrc;
>+ }
>+ if (v6_m_spec->pdst) {
>+ match->key.tp.dst = v6_spec->pdst;
>+ match->mask.tp.dst = v6_m_spec->pdst;
>+ }
>+ if (v6_m_spec->psrc ||
>+ v6_m_spec->pdst) {
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
>+ offsetof(struct ethtool_rx_flow_key, tp);
>+ }
>+ if (v6_m_spec->tclass) {
>+ match->key.ip.tos = v6_spec->tclass;
>+ match->mask.ip.tos = v6_m_spec->tclass;
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
>+ offsetof(struct ethtool_rx_flow_key, ip);
>+ }
>+ }
>+ break;
>+ }
>+
>+ switch (fs->flow_type & ~FLOW_EXT) {
>+ case TCP_V4_FLOW:
>+ case TCP_V6_FLOW:
>+ match->key.basic.ip_proto = IPPROTO_TCP;
>+ break;
>+ case UDP_V4_FLOW:
>+ case UDP_V6_FLOW:
>+ match->key.basic.ip_proto = IPPROTO_UDP;
>+ break;
>+ }
>+ match->mask.basic.ip_proto = 0xff;
>+
>+ match->dissector.used_keys |= FLOW_DISSECTOR_KEY_BASIC;
>+ match->dissector.offset[FLOW_DISSECTOR_KEY_BASIC] =
>+ offsetof(struct ethtool_rx_flow_key, basic);
>+
>+ /* ethtool_rx supports only one single action per rule. */
>+ if (flow_action_init(&rule->action, 1) < 0)
>+ goto err_action;
>+
>+ act = &rule->action.keys[0];
>+ switch (fs->ring_cookie) {
>+ case RX_CLS_FLOW_DISC:
>+ act->id = FLOW_ACTION_KEY_DROP;
>+ break;
>+ case RX_CLS_FLOW_WAKE:
>+ act->id = FLOW_ACTION_KEY_WAKE;
>+ break;
>+ default:
>+ act->id = FLOW_ACTION_KEY_QUEUE;
>+ act->queue_index = fs->ring_cookie;
>+ break;
>+ }
>+
>+ return rule;
>+
>+err_action:
>+ kfree(match);
>+err_match:
>+ kfree(rule);
>+ return NULL;
>+}
>+EXPORT_SYMBOL(ethtool_rx_flow_rule);
>+
>+void ethtool_rx_flow_rule_free(struct flow_rule *rule)
>+{
>+ kfree((struct flow_match *)rule->match.dissector);
Ouch. I wonder if it cannot be stored rather in some rule->priv or
something.
On alloc, you can have a helper to allocate both:
***
struct flow_rule {
...
unsigned long priv[0];
};
struct flow_rule *flow_rule_alloc(size_t priv_size)
{
return kzalloc(sizeof(struct flow_rule) + priv_size, ...);
}
>+ flow_action_free(&rule->action);
>+ kfree(rule);
>+}
>+EXPORT_SYMBOL(ethtool_rx_flow_rule_free);
>+
> /**
> * __skb_flow_get_ports - extract the upper layer ports and return them
> * @skb: sk_buff to extract the ports from
>--
>2.11.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox