On 31/05/2017 03:20, Joe Stringer wrote:
On 28 May 2017 at 04:59, Roi Dayan <[email protected]> wrote:Add tc module to expose tc operations to be used by other modules. Move some tc related functions from netdev-linux.c to tc.c This patch doesn't change any functionality. Signed-off-by: Paul Blakey <[email protected]> Signed-off-by: Roi Dayan <[email protected]> ---Same comment about 2 signoffs but no co-author tag. This could do with a rebase. In future please do this just before you submit a series, I spent a few minutes trying to figure out how I could apply this series to review it. I have a few other very trivial, nitpicking comments below so otherwise here's my ack: Acked-by: Joe Stringer <[email protected]>lib/automake.mk | 2 +1 lib/netdev-linux.c | 99 +--------------------------------------------- lib/tc.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/tc.h | 34 ++++++++++++++++ 4 files changed, 151 insertions(+), 98 deletions(-) create mode 100644 lib/tc.c create mode 100644 lib/tc.h diff --git a/lib/automake.mk b/lib/automake.mk index faace79..3d57610 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -352,6 +352,8 @@ if LINUX lib_libopenvswitch_la_SOURCES += \ lib/dpif-netlink.c \ lib/dpif-netlink.h \ + lib/tc.h \ + lib/tc.c \ lib/if-notifier.c \ lib/if-notifier.h \ lib/netdev-linux.c \These should be alphabetically organised.
ok
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 4fc3f6b..7a0517b 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -29,8 +29,6 @@ #include <linux/types.h> #include <linux/ethtool.h> #include <linux/mii.h> -#include <linux/pkt_cls.h> -#include <linux/pkt_sched.h> #include <linux/rtnetlink.h>If you're refactoring these #includes to tc.h, then linux/rtnetlink.h could go with them. That said, I'm not sure whether there's a preference in OVS to keep them in the .c files or put them in the .h.
ok
#include <linux/sockios.h> #include <sys/types.h> @@ -74,6 +72,7 @@ #include "unaligned.h" #include "openvswitch/vlog.h" #include "util.h" +#include "tc.h" VLOG_DEFINE_THIS_MODULE(netdev_linux); @@ -434,22 +433,14 @@ static const struct tc_ops *const tcs[] = { NULL }; -static unsigned int tc_make_handle(unsigned int major, unsigned int minor); -static unsigned int tc_get_major(unsigned int handle); -static unsigned int tc_get_minor(unsigned int handle); - static unsigned int tc_ticks_to_bytes(unsigned int rate, unsigned int ticks); static unsigned int tc_bytes_to_ticks(unsigned int rate, unsigned int size); static unsigned int tc_buffer_per_jiffy(unsigned int rate); -static struct tcmsg *tc_make_request(int ifindex, int type, - unsigned int flags, struct ofpbuf *); static struct tcmsg *netdev_linux_tc_make_request(const struct netdev *, int type, unsigned int flags, struct ofpbuf *); -static int tc_transact(struct ofpbuf *request, struct ofpbuf **replyp); -static int tc_add_del_ingress_qdisc(int ifindex, bool add); static int tc_add_policer(struct netdev *, uint32_t kbits_rate, uint32_t kbits_burst); @@ -4643,44 +4634,6 @@ static double ticks_per_s; */ static unsigned int buffer_hz; -/* Returns tc handle 'major':'minor'. */ -static unsigned int -tc_make_handle(unsigned int major, unsigned int minor) -{ - return TC_H_MAKE(major << 16, minor); -} - -/* Returns the major number from 'handle'. */ -static unsigned int -tc_get_major(unsigned int handle) -{ - return TC_H_MAJ(handle) >> 16; -} - -/* Returns the minor number from 'handle'. */ -static unsigned int -tc_get_minor(unsigned int handle) -{ - return TC_H_MIN(handle); -} - -static struct tcmsg * -tc_make_request(int ifindex, int type, unsigned int flags, - struct ofpbuf *request) -{ - struct tcmsg *tcmsg; - - ofpbuf_init(request, 512); - nl_msg_put_nlmsghdr(request, sizeof *tcmsg, type, NLM_F_REQUEST | flags); - 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 struct tcmsg * netdev_linux_tc_make_request(const struct netdev *netdev, int type, unsigned int flags, struct ofpbuf *request) @@ -4696,56 +4649,6 @@ netdev_linux_tc_make_request(const struct netdev *netdev, int type, return tc_make_request(ifindex, type, flags, request); } -static int -tc_transact(struct ofpbuf *request, struct ofpbuf **replyp) -{ - int error = nl_transact(NETLINK_ROUTE, request, replyp); - ofpbuf_uninit(request); - return error; -} - -/* Adds or deletes a root ingress qdisc on 'netdev'. We use this for - * policing configuration. - * - * This function is equivalent to running the following when 'add' is true: - * /sbin/tc qdisc add dev <devname> handle ffff: ingress - * - * This function is equivalent to running the following when 'add' is false: - * /sbin/tc qdisc del dev <devname> handle ffff: ingress - * - * The configuration and stats may be seen with the following command: - * /sbin/tc -s qdisc show dev <devname> - * - * Returns 0 if successful, otherwise a positive errno value. - */ -static int -tc_add_del_ingress_qdisc(int ifindex, bool add) -{ - struct ofpbuf request; - struct tcmsg *tcmsg; - int error; - int type = add ? RTM_NEWQDISC : RTM_DELQDISC; - int flags = add ? NLM_F_EXCL | NLM_F_CREATE : 0; - - tcmsg = tc_make_request(ifindex, type, flags, &request); - tcmsg->tcm_handle = tc_make_handle(0xffff, 0); - tcmsg->tcm_parent = TC_H_INGRESS; - nl_msg_put_string(&request, TCA_KIND, "ingress"); - nl_msg_put_unspec(&request, TCA_OPTIONS, NULL, 0); - - error = tc_transact(&request, NULL); - if (error) { - /* If we're deleting the qdisc, don't worry about some of the - * error conditions. */ - if (!add && (error == ENOENT || error == EINVAL)) { - return 0; - } - return error; - } - - return 0; -} - /* Adds a policer to 'netdev' with a rate of 'kbits_rate' and a burst size * of 'kbits_burst'. * diff --git a/lib/tc.c b/lib/tc.c new file mode 100644 index 0000000..644f30c --- /dev/null +++ b/lib/tc.c @@ -0,0 +1,114 @@ +/* + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <config.h> +#include "tc.h" +#include <errno.h> +#include "netlink-socket.h" +#include "netlink.h" +#include "openvswitch/vlog.h" +#include "openvswitch/ofpbuf.h"Header includes are typically defined as: * config include * include header with same name as file * system headers * OVS headers Which you've done, but also within each category above they are typically ordered alphabetically.
ok
+ +VLOG_DEFINE_THIS_MODULE(tc); + +/* Returns tc handle 'major':'minor'. */ +unsigned int +tc_make_handle(unsigned int major, unsigned int minor) +{ + return TC_H_MAKE(major << 16, minor); +} + +/* Returns the major number from 'handle'. */ +unsigned int +tc_get_major(unsigned int handle) +{ + return TC_H_MAJ(handle) >> 16; +} + +/* Returns the minor number from 'handle'. */ +unsigned int +tc_get_minor(unsigned int handle) +{ + return TC_H_MIN(handle); +} + +struct tcmsg * +tc_make_request(int ifindex, int type, unsigned int flags, + struct ofpbuf *request) +{ + struct tcmsg *tcmsg; + + ofpbuf_init(request, 512); + nl_msg_put_nlmsghdr(request, sizeof *tcmsg, type, NLM_F_REQUEST | flags); + 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; +} + +int +tc_transact(struct ofpbuf *request, struct ofpbuf **replyp) +{ + int error = nl_transact(NETLINK_ROUTE, request, replyp); + ofpbuf_uninit(request); + return error; +} + +/* Adds or deletes a root ingress qdisc on device with specified ifindex. + * + * This function is equivalent to running the following when 'add' is true: + * /sbin/tc qdisc add dev <devname> handle ffff: ingress + * + * This function is equivalent to running the following when 'add' is false: + * /sbin/tc qdisc del dev <devname> handle ffff: ingress + * + * Where dev <devname> is the device with specified ifindex name. + * + * The configuration and stats may be seen with the following command: + * /sbin/tc -s qdisc show dev <devname> + * + * Returns 0 if successful, otherwise a positive errno value. + */ +int +tc_add_del_ingress_qdisc(int ifindex, bool add) +{ + struct ofpbuf request; + struct tcmsg *tcmsg; + int error; + int type = add ? RTM_NEWQDISC : RTM_DELQDISC; + int flags = add ? NLM_F_EXCL | NLM_F_CREATE : 0; + + tcmsg = tc_make_request(ifindex, type, flags, &request); + tcmsg->tcm_handle = tc_make_handle(0xffff, 0); + tcmsg->tcm_parent = TC_H_INGRESS; + nl_msg_put_string(&request, TCA_KIND, "ingress"); + nl_msg_put_unspec(&request, TCA_OPTIONS, NULL, 0); + + error = tc_transact(&request, NULL); + if (error) { + /* If we're deleting the qdisc, don't worry about some of the + * error conditions. */ + if (!add && (error == ENOENT || error == EINVAL)) { + return 0; + } + return error; + } + + return 0; +} diff --git a/lib/tc.h b/lib/tc.h new file mode 100644 index 0000000..dc79208 --- /dev/null +++ b/lib/tc.h @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017 Nicira, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef TC_H +#define TC_H 1 + +#include <linux/rtnetlink.h> +#include <linux/pkt_cls.h> +#include <linux/pkt_sched.h> +#include "odp-netlink.h" +#include "netlink-socket.h"Again, alphabetic ordering in each category: system headers and OVS headers.+ +unsigned int tc_make_handle(unsigned int major, unsigned int minor); +unsigned int tc_get_major(unsigned int handle); +unsigned int tc_get_minor(unsigned int handle); +struct tcmsg *tc_make_request(int ifindex, int type, + unsigned int flags, struct ofpbuf *); +int tc_transact(struct ofpbuf *request, struct ofpbuf **replyp); +int tc_add_del_ingress_qdisc(int ifindex, bool add); + +#endif /* tc.h */ -- 2.7.4
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
