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. > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
