Hi Joe,
Thanks for your comprehensive feedback (on this and other patches), I've
added some inline comments.
As for coding style / duplicate code / checkpatch / name changes / etc
comments, we already started fixing it.
Thanks,
Paul Blakey.
On 14/11/2016 22:47, Joe Stringer wrote:
There are various style variations that are unusual in this whole
series (compared against the standard OVS style), I've pointed out a
few of them in this patch along with some specific feedback on this
patch. It's not comprehensive though, there is too much for me to
comment on.
On 1 November 2016 at 07:53, Paul Blakey <[email protected]> wrote:
Add tc interface in order to send/recv data from/to the HW.
The kerenl side of tc will pass the tc messages to the driver and back.
Signed-off-by: Paul Blakey <[email protected]>
Signed-off-by: Shahar Klein <[email protected]>
As far as I can tell (that is, looking at the header file), this is
exclusively flower-related tc. Please move it to lib/tc-flower.[ch].
---
lib/automake.mk | 2 +
lib/tc.c | 906 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
lib/tc.h | 86 ++++++
3 files changed, 994 insertions(+)
create mode 100644 lib/tc.c
create mode 100644 lib/tc.h
diff --git a/lib/automake.mk b/lib/automake.mk
index f00c8ae..be2c8eb 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -341,6 +341,8 @@ if LINUX
lib_libopenvswitch_la_SOURCES += \
lib/dpif-netlink.c \
lib/dpif-netlink.h \
+ lib/tc.h \
+ lib/tc.c \
lib/dpif-hw-acc.c \
lib/dpif-hw-acc.h \
lib/if-notifier.c \
diff --git a/lib/tc.c b/lib/tc.c
new file mode 100644
index 0000000..2f8acb8
--- /dev/null
+++ b/lib/tc.c
@@ -0,0 +1,906 @@
+
There seems to be a license missing here. Typically people attach the
Apache2 license with copyright assigned to themselves or their
employer, just like the other files. (If there's refactoring, please
bring the existing copyright with it as well).
+#include <config.h>
+
+#include <errno.h>
+#include <linux/rtnetlink.h>
+#include <net/if.h>
+#include <linux/tc_act/tc_gact.h>
+#include <linux/tc_act/tc_mirred.h>
+#include <linux/gen_stats.h>
+#include "timeval.h"
+#include "netlink-socket.h"
+#include "netlink.h"
+#include "ofpbuf.h"
+#include "rtnetlink.h"
+#include "openvswitch/vlog.h"
+#include "tc.h"
+#include "util.h"
+
+#ifndef __LINUX_TC_VLAN_H
+#define __LINUX_TC_VLAN_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_VLAN 12
+
+#define TCA_VLAN_ACT_POP 1
+#define TCA_VLAN_ACT_PUSH 2
+#define TCA_VLAN_ACT_MODIFY 3
+
+struct tc_vlan {
+ tc_gen;
+ int v_action;
+};
+
+enum {
+ TCA_VLAN_UNSPEC,
+ TCA_VLAN_TM,
+ TCA_VLAN_PARMS,
+ TCA_VLAN_PUSH_VLAN_ID,
+ TCA_VLAN_PUSH_VLAN_PROTOCOL,
+ TCA_VLAN_PAD,
+ TCA_VLAN_PUSH_VLAN_PRIORITY,
+ __TCA_VLAN_MAX,
+};
+#define TCA_VLAN_MAX (__TCA_VLAN_MAX - 1)
+
+#endif
Why aren't you just including <linux/tc_act/tc_vlan.h>?
+
+
+bool SKIP_HW = false;
What is the granularity of this configuration option supposed to be?
This variable corresponds to flower classifier skip_hw/skip_sw flags, so
the granularity would have been true/false. Right now this variable changes
according to the bridge name. We thought about moving this to vsctl
other_config variable.
Its purpose was to compare HW offload vs handling in tc/software.
+VLOG_DEFINE_THIS_MODULE(tc);
+
+/* Returns tc handle 'major':'minor'. */
+static unsigned int
+tc_make_handle(unsigned int major, unsigned int minor)
+{
+ return TC_H_MAKE(major << 16, minor);
+}
+
+static struct tcmsg *
+hw_tc_make_request(int ifindex, int type, unsigned int flags,
+ struct ofpbuf *request)
+{
+ struct tcmsg *tcmsg;
+
+ ofpbuf_init(request, 512);
+
+ struct nlmsghdr *nlmsghdr;
+
+ ovs_assert(request->size == 0);
+
+ nl_msg_reserve(request, NLMSG_HDRLEN + sizeof *tcmsg);
+ nlmsghdr = nl_msg_put_uninit(request, NLMSG_HDRLEN);
+ nlmsghdr->nlmsg_len = 0;
+ nlmsghdr->nlmsg_type = type;
+ nlmsghdr->nlmsg_flags = NLM_F_REQUEST | flags;
+ nlmsghdr->nlmsg_seq = 0;
+ nlmsghdr->nlmsg_pid = 0;
+
+ tcmsg = ofpbuf_put_zeros(request, sizeof *tcmsg);
+ tcmsg->tcm_family = AF_UNSPEC;
+ tcmsg->tcm_ifindex = ifindex;
+ /* Caller should fill in tcmsg->tcm_handle. */
+ /* Caller should fill in tcmsg->tcm_parent. */
+
+ return tcmsg;
+}
+
+static int
+tc_transact(struct ofpbuf *request, struct ofpbuf **replyp)
+{
+ int error = nl_transact(NETLINK_ROUTE, request, replyp);
+
+ ofpbuf_uninit(request);
+ return error;
+}
The above three functions look like mostly copy-paste from a few
functions in lib/netdev-linux.c. Please consider factoring out that
code, for example into a generic tc lib/tc.[ch] for functions like
making requests, transactions, and other non-flower related pieces.
lib/netdev-linux.c can reuse those.
You're right, some code reuse is because we wanted minimal changes to
existing files, we'll do that.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
