On Thu, Oct 16, 2014 at 11:38:20AM -0700, Pravin B Shelar wrote:
> Following patch adds support for userspace tunneling. Tunneling
> needs three more component first is routing table which is configured by
> caching kernel routes and second is ARP cache which build automatically
> by snooping arp. And third is tunnel protocol table which list all
> listening protocols which is populated by vswitchd as tunnel ports
> are added.
>
> Tunneling works as follows:
> On packet receive vswitchd check if this packet is targeted to tunnel
> port. If it is then vswitchd inserts tunnel pop action which pops
> header and sends packet to tunnel port.
> On packet xmit rather than generating Set tunnel action it generate
> tunnel push action which has tunnel header data. datapath can use
> tunnel-push action data to generate header for each packet and
> forward this packet to output port. Since tunnel-push action
> contains most of packet header it need to lookup routing table and
> arp table to build this action.
>
> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
I think that struct ovs_action_push_tnl is meant to be userspace-only so
far. In openvswitch.h, should the struct definition itself be enclosed
in #ifndef __KERNEL__...#endif?
(But I find myself wondering whether we should have a userspace-only
extension mechanism for openvswitch.h.)
> +static int
> +push_tunnel_action(const struct dp_netdev *dp,
> + const struct nlattr *attr,
> + struct dpif_packet **packets, int cnt)
> +{
> + struct dp_netdev_port *tun_port;
> + struct ovs_action_push_tnl *data;
> +
> + data = (struct ovs_action_push_tnl *) nl_attr_get(attr);
The cast above shouldn't really be necessary, at least if you mark
'data' as const.
> +static void
> +dp_netdev_clone_pkt_batch(struct dpif_packet **tnl_pkt,
> + struct dpif_packet **packets, int cnt)
> +{
> + int i;
> +
> + for (i = 0; i < cnt; i++) {
> + struct dpif_packet *inner_pkt;
> +
> + inner_pkt = dpif_packet_clone(packets[i]);
> + tnl_pkt[i] = inner_pkt;
The lines above are a long way to write "tnl_pkt[i] =
dpif_packet_clone(packets[i]);".
> + }
> +}
> +
> static void
> dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
> const struct nlattr *a, bool may_steal)
> @@ -2816,6 +2862,60 @@ dp_execute_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> }
> break;
>
> + case OVS_ACTION_ATTR_TUNNEL_PUSH:
> + if (*depth < MAX_RECIRC_DEPTH) {
> + struct dpif_packet *tnl_pkt[NETDEV_MAX_RX_BATCH];
> + int err;
> +
> + if (!may_steal) {
> + dp_netdev_clone_pkt_batch(tnl_pkt, packets, cnt);
> + packets = tnl_pkt;
> + }
> +
> + err = push_tunnel_action(dp, a, packets, cnt);
> + if (err) {
> + dp_netdev_drop_packets(tnl_pkt, cnt, !may_steal);
> + break;
> + }
> +
> + (*depth)++;
> + dp_netdev_input(pmd, packets, cnt);
> + (*depth)--;
> + return;
> + }
> + break;
> +
> + case OVS_ACTION_ATTR_TUNNEL_POP:
> + if (*depth < MAX_RECIRC_DEPTH) {
> + odp_port_t portno = u32_to_odp(nl_attr_get_u32(a));
> +
> + p = dp_netdev_lookup_port(dp, portno);
> + if (p) {
> + struct dpif_packet *tnl_pkt[NETDEV_MAX_RX_BATCH];
> + int err;
> +
> + if (!may_steal) {
> + dp_netdev_clone_pkt_batch(tnl_pkt, packets, cnt);
> + packets = tnl_pkt;
> + }
> +
> + err = netdev_pop_header(p->netdev, packets, cnt);
> + if (!err) {
> +
> + for (i = 0; i < cnt; i++) {
> + packets[i]->md.in_port.odp_port = portno;
> + }
> +
> + (*depth)++;
> + dp_netdev_input(pmd, packets, cnt);
> + (*depth)--;
> + return;
> + }
> + dp_netdev_drop_packets(tnl_pkt, cnt, !may_steal);
The OVS_ACTION_ATTR_TUNNEL_POP case here handles the success case inside
an 'if' statement; the OVS_ACTION_ATTR_TUNNEL_PUSH handles the failure
case inside an 'if' statement. I think that either approach is fine but
I'd find the code more straightforward if a consistent approach were used.
We now have multiple cases of code that checks and increments the depth
and calls dp_netdev_input() and reduces the depth. I wonder whether we
could encapsulate that in a neat helper function, or just put it into
dp_netdev_input() itself.
We usually put the return type and function name on separate lines in
userspace:
> +bool dpif_support_tnl_push_pop(const struct dpif *dpif)
> +{
> + return !strcmp(dpif->dpif_class->type, "netdev") ||
> + !strcmp(dpif->dpif_class->type, "dummy");
> +}
We usually give the header guard the same name as the header file (this
is in a file named gre.h):
> +#ifndef NETDEV_GRE_H
> +#define NETDEV_GRE_H 1
> +
This header file doesn't require most of the headers below:
> +#include <stddef.h>
> +#include <stdint.h>
> +#include "list.h"
> +#include "packets.h"
> +#include "util.h"
> +#include "netdev-dpdk.h"
We don't usually add extern "C" unless someone asks for them:
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* GRE protocol header */
> +struct gre_base_hdr {
> + ovs_be16 flags;
> + ovs_be16 protocol;
This __attribute__ is going to cause trouble on MSVC:
> +}__attribute__((aligned(4)));
> +
> +#define GRE_HEADER_SECTION 4
> +
It's pretty unusual for us to put htons() into value definitions:
> +#define GRE_CSUM htons(0x8000)
> +#define GRE_ROUTING htons(0x4000)
> +#define GRE_KEY htons(0x2000)
> +#define GRE_SEQ htons(0x1000)
> +#define GRE_STRICT htons(0x0800)
> +#define GRE_REC htons(0x0700)
> +#define GRE_FLAGS htons(0x00F8)
> +#define GRE_VERSION htons(0x0007)
Did you consider putting the GRE definitions into packets.h?
I need to restart reading the code at netdev-vport.c.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
