[ovs-dev] [PATCH V9 02/31] tc: Introduce tc module
Joe Stringer
joe at ovn.org
Wed May 31 00:20:27 UTC 2017
On 28 May 2017 at 04:59, Roi Dayan <roid at mellanox.com> 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 <paulb at mellanox.com>
> Signed-off-by: Roi Dayan <roid at mellanox.com>
> ---
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 <joe at ovn.org>
> 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.
> 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.
> #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.
> +
> +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
>
More information about the dev
mailing list