On Fri, Oct 24, 2014 at 9:43 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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.)
>
I will give a shot. Let me know if you already have idea.
>> +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.
>
ok.
>> +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]);".
>
ok.
>
>> + }
>> +}
>> +
>> 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.
>
ok.
> 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");
>> +}
>
right.
> 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:
>
ok, I will use packed attribute.
>> +}__attribute__((aligned(4)));
>> +
>> +#define GRE_HEADER_SECTION 4
>> +
>
> It's pretty unusual for us to put htons() into value definitions:
ok I will fix it.
>> +#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?
>
ok, I will move gre and vxlan header definitions to 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
