[ovs-dev] [PATCH ovs V8 02/26] netdev: Adding a new netdev api to be used for offloading flows
Roi Dayan
roid at mellanox.com
Mon May 15 06:28:31 UTC 2017
On 08/05/2017 15:28, Simon Horman wrote:
> On Wed, May 03, 2017 at 06:07:53PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb at mellanox.com>
>
> Please add some text to the changelog.
>
>>
>> Signed-off-by: Paul Blakey <paulb at mellanox.com>
>> Reviewed-by: Roi Dayan <roid at mellanox.com>
>> Reviewed-by: Simon Horman <simon.horman at netronome.com>
>
> ...
>
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> new file mode 100644
>> index 0000000..eb5e79a
>> --- /dev/null
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Copyright (c) 2016 Mellanox Technologies, Ltd.
>> + *
>> + * 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 "netdev-tc-offloads.h"
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <arpa/inet.h>
>> +#include <inttypes.h>
>> +#include <linux/filter.h>
>> +#include <linux/gen_stats.h>
>> +#include <linux/if_ether.h>
>> +#include <linux/if_tun.h>
>> +#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>
>> +#include <linux/sockios.h>
>> +#include <sys/types.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/socket.h>
>> +#include <sys/utsname.h>
>> +#include <netpacket/packet.h>
>> +#include <net/if.h>
>> +#include <net/if_arp.h>
>> +#include <net/if_packet.h>
>> +#include <net/route.h>
>> +#include <netinet/in.h>
>> +#include <poll.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +
>> +#include "coverage.h"
>> +#include "dp-packet.h"
>> +#include "dpif-netlink.h"
>> +#include "dpif-netdev.h"
>> +#include "openvswitch/dynamic-string.h"
>> +#include "fatal-signal.h"
>> +#include "hash.h"
>> +#include "openvswitch/hmap.h"
>> +#include "netdev-provider.h"
>> +#include "netdev-vport.h"
>> +#include "netlink-notifier.h"
>> +#include "netlink-socket.h"
>> +#include "netlink.h"
>> +#include "openvswitch/ofpbuf.h"
>> +#include "openflow/openflow.h"
>> +#include "ovs-atomic.h"
>> +#include "packets.h"
>> +#include "poll-loop.h"
>> +#include "rtnetlink.h"
>> +#include "openvswitch/shash.h"
>> +#include "netdev-provider.h"
>> +#include "openvswitch/match.h"
>> +#include "openvswitch/vlog.h"
>> +#include "tc.h"
>
> Are all of the above headers needed for what follows?
> There seems to be a lot of #includes above.
probably not. was started as copy paste to avoid compile errors and we
forgot to clean it.
>
>> +VLOG_DEFINE_THIS_MODULE(netdev_tc_offloads);
>> +
>> +int
>> +netdev_tc_flow_flush(struct netdev *netdev OVS_UNUSED)
>> +{
>> + return EOPNOTSUPP;
>> +}
>> +
>> +int
>> +netdev_tc_flow_dump_create(struct netdev *netdev,
>> + struct netdev_flow_dump **dump_out)
>> +{
>> + struct netdev_flow_dump *dump = xzalloc(sizeof *dump);
>> +
>> + dump->netdev = netdev_ref(netdev);
>> +
>> + *dump_out = dump;
>> +
>> + return 0;
>> +}
>> +
>> +int
>> +netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)
>> +{
>> + netdev_close(dump->netdev);
>> + free(dump);
>> +
>> + return 0;
>> +}
>> +
>> +bool
>> +netdev_tc_flow_dump_next(struct netdev_flow_dump *dump OVS_UNUSED,
>> + struct match *match OVS_UNUSED,
>> + struct nlattr **actions OVS_UNUSED,
>> + struct dpif_flow_stats *stats OVS_UNUSED,
>> + ovs_u128 *ufid OVS_UNUSED,
>> + struct ofpbuf *rbuffer OVS_UNUSED,
>> + struct ofpbuf *wbuffer OVS_UNUSED)
>> +{
>> + return false;
>> +}
>> +
>> +int
>> +netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED,
>> + struct match *match OVS_UNUSED,
>> + struct nlattr *actions OVS_UNUSED,
>> + size_t actions_len OVS_UNUSED,
>> + struct dpif_flow_stats *stats OVS_UNUSED,
>> + const ovs_u128 *ufid OVS_UNUSED,
>
> Here and above ufid follows stats.
>
>> + struct offload_info *info OVS_UNUSED)
>> +{
>> + return EOPNOTSUPP;
>> +}
>> +
>> +int
>> +netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>> + struct match *match OVS_UNUSED,
>> + struct nlattr **actions OVS_UNUSED,
>> + const ovs_u128 *ufid OVS_UNUSED,
>> + struct dpif_flow_stats *stats OVS_UNUSED,
>
> But here the order is reversed.
>
> I always think that when a reviewer asks for entries to be sorted they
> have run out of things to say. But nonetheless it would be slightly
> nicer if the order was consistent unless there is a reason for it not to
> be.
I agree. In one of the submissions I fixed the order but probably didn't
notice this one. thanks.
>
>> + struct ofpbuf *buf OVS_UNUSED)
>> +{
>> + return EOPNOTSUPP;
>> +}
>> +
>> +int
>> +netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>> + const ovs_u128 *ufid OVS_UNUSED,
>> + struct dpif_flow_stats *stats OVS_UNUSED)
>> +{
>> + return EOPNOTSUPP;
>> +}
>> +
>> +int
>> +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED)
>> +{
>> + return 0;
>> +}
>> +
>
> There is a trailing blank line above.
ok
>
>> diff --git a/lib/netdev-tc-offloads.h b/lib/netdev-tc-offloads.h
>> new file mode 100644
>> index 0000000..76b7938
>> --- /dev/null
>> +++ b/lib/netdev-tc-offloads.h
>> @@ -0,0 +1,42 @@
>
>> +int netdev_tc_flow_put(struct netdev *, struct match *,
>> + struct nlattr *actions, size_t actions_len,
>> + const ovs_u128 *, struct offload_info *,
>> + struct dpif_flow_stats *);
>
> The declaration of netdev_tc_flow_put() does not match its definition.
>
> ...
>
ok. probably happened from all the rebases. i'll fix it.
More information about the dev
mailing list