On 31/05/2017 03:50, Joe Stringer wrote:
On 28 May 2017 at 04:59, Roi Dayan <[email protected]> wrote:
Add tc helper functions to query and manipulate the flower classifier.
Signed-off-by: Paul Blakey <[email protected]>
Signed-off-by: Roi Dayan <[email protected]>
---
Again is this co-authored? utilities/checkpatch.py checks for stuff like this.
I didn't go through all of the enums and make sure they're all
covered, correct types, etc.
<snip>
+ [TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK] = { .type = NL_A_U32, .optional = true,
},
+ [TCA_FLOWER_KEY_ENC_IPV4_DST_MASK] = { .type = NL_A_U32, .optional = true,
},
Line lengths.
ok
<snip>
+static void
+nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
+{
+ unsigned long long int lastuse = tm->lastuse * 10;
Where does the 10 come from? What does it mean? Should we have a #define for it?
we want to convert here jiffies to ms and used some default value.
as Flavio suggested maybe do it in a macro?
later I saw in netdev-linux.c the functions read_psched() and
tc_ticks_to_bytes(). can do a followup commit to refactor those
somewhere else and use them in tc.c as well.
<snip>
+ stats_basic = stats_attrs[TCA_STATS_BASIC];
+ bs = nl_attr_get_unspec(stats_basic, sizeof *bs);
+
+ stats->n_packets.lo = bs->packets;
+ stats->n_packets.hi = 0;
+ stats->n_bytes.hi = bs->bytes >> 32;
+ stats->n_bytes.lo = bs->bytes & 0x00000000FFFFFFFF;
Should this use put_32aligned_u64()?
yes. wanted to do that as followup commit later to avoid too
many changes. I can merge it now though.
+
+ return 0;
+}
+
+#define TCA_ACT_MIN_PRIO 1
Stray define - Shouldn't this be in linux/pkt_cls.h?
it's not in pkt_cls.h, we only have TCA_ACT_MAX_PRIO
We can also see in act_api.c in kernel that they loop from 1
and don't use a definition.
we added a local definition for readability.
<snip>
+ if (!nl_policy_parse(reply, NLMSG_HDRLEN + sizeof *tc,
+ tca_policy, ta, ARRAY_SIZE(ta))) {
+ VLOG_ERR_RL(&error_rl, "failed to parse tca policy");
+ return EPROTO;
+ }
+
+ kind = nl_attr_get_string(ta[TCA_KIND]);
+ if (strcmp(kind, "flower")) {
+ VLOG_ERR_RL(&error_rl, "failed to parse filter: not flower");
Printing the filter kind in the log message might be useful for
debugging purposes.
ok
+
+ tcmsg = tc_make_request(ifindex, RTM_GETTFILTER, NLM_F_DUMP, &request);
+ tcmsg->tcm_parent = tc_make_handle(0xffff, 0);
Do TC people usually define handles in terms of these specific
integers? I wonder if a subsequent follow-up patch would improve
readability by #defining these tc_make_handle(0xffff, 0) numbers to
show what they actually mean - like ingress, root.
currently we copied it from existing functions that already set
tcm_parent like this.
In tc we can use the word 'ingress' which then in the code is
translating to:
req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
we can do a followup patch to do this as well or even on this add
another macro.
+int
+tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
+ struct tc_flower *flower)
+{
+ struct ofpbuf request;
+ struct tcmsg *tcmsg;
+ struct ofpbuf *reply;
+ int error = 0;
+ size_t basic_offset;
+ uint16_t eth_type = (OVS_FORCE uint16_t) flower->key.eth_type;
Why does this need forcing?
as eth_type is ove_be16 but tc_make_handle wants unsigned int.
we get a compilation warning from sparse about incorrect type.
this is to avoid sparse from failing.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
