On 16 November 2016 at 08:22, Paul Blakey <[email protected]> wrote: > 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, I look forward to v2. > 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. By granularity, I meant how widely is it supposed to apply? It's currently set in dpif_hw_acc_open() and port_add() - and only ever set true. Is this parameter intended to be specified in the database by the user, or is it more of a debug thing? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
