*, On May 1, 2013 8:49 AM, <dev-requ...@openvswitch.org> wrote:
> Send dev mailing list submissions to > dev@openvswitch.org > > To subscribe or unsubscribe via the World Wide Web, visit > http://openvswitch.org/mailman/listinfo/dev > or, via email, send a message with subject or body 'help' to > dev-requ...@openvswitch.org > > You can reach the person managing the list at > dev-ow...@openvswitch.org > > When replying, please edit your Subject line so it is more specific > than "Re: Contents of dev digest..." > > > Today's Topics: > > 1. [PATCH v2 2/2] datapath: Move vport init to First port > create. (Pravin B Shelar) > 2. Re: [PATCH v2 1/2] tunneling: Remove struct tnl_vport and > tnl_ops. (Jesse Gross) > 3. Re: [PATCH v2 2/2] datapath: Move vport init to First port > create. (Jesse Gross) > 4. Re: [PATCH v2 1/2] tunneling: Remove struct tnl_vport and > tnl_ops. (Pravin Shelar) > > > ---------------------------------------------------------------------- > > Message: 1 > Date: Tue, 30 Apr 2013 16:21:09 -0700 > From: Pravin B Shelar <pshe...@nicira.com> > Subject: [ovs-dev] [PATCH v2 2/2] datapath: Move vport init to First > port create. > To: dev@openvswitch.org > Message-ID: <1367364069-4964-1-git-send-email-pshe...@nicira.com> > > vport->init and exit() functios are defined by gre and netdev vport > only and both can be moved to first port create. > > Following patch does same, it moves vport init to respectve vport > create and get rid of vport->init() and vport->exit() fnctions. > > Signed-off-by: Pravin B Shelar <pshe...@nicira.com> > --- > v1-v2: > No Change. > --- > datapath/vport-gre.c | 49 +++++++++++++++++++++++----------- > datapath/vport-internal_dev.c | 1 - > datapath/vport-netdev.c | 19 ++++++++++--- > datapath/vport.c | 58 > ++++------------------------------------ > datapath/vport.h | 11 +------- > 5 files changed, 54 insertions(+), 84 deletions(-) > > diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c > index 2db4934..59e22be 100644 > --- a/datapath/vport-gre.c > +++ b/datapath/vport-gre.c > @@ -290,16 +290,15 @@ static const struct net_protocol > gre_protocol_handlers = { > #endif > }; > > -static bool inited; > - > +static int gre_ports; > static int gre_init(void) > { > int err; > > - if (inited) > + gre_ports++; > + if (gre_ports > 1) > return 0; > > - inited = true; > err = inet_add_protocol(&gre_protocol_handlers, IPPROTO_GRE); > if (err) > pr_warn("cannot register gre protocol handler\n"); > @@ -309,11 +308,10 @@ static int gre_init(void) > > static void gre_exit(void) > { > - if (!inited) > + gre_ports--; > + if (gre_ports > 0) > return; > > - inited = false; > - > inet_del_protocol(&gre_protocol_handlers, IPPROTO_GRE); > } > > @@ -327,10 +325,17 @@ static struct vport *gre_create(const struct > vport_parms *parms) > struct net *net = ovs_dp_get_net(parms->dp); > struct ovs_net *ovs_net; > struct vport *vport; > + int err; > + > + err = gre_init(); > + if (err) > + return ERR_PTR(err); > > ovs_net = net_generic(net, ovs_net_id); > - if (ovsl_dereference(ovs_net->vport_net.gre_vport)) > - return ERR_PTR(-EEXIST); > + if (ovsl_dereference(ovs_net->vport_net.gre_vport)) { > + vport = ERR_PTR(-EEXIST); > + goto error; > + } > > vport = ovs_vport_alloc(IFNAMSIZ, &ovs_gre_vport_ops, parms); > if (IS_ERR(vport)) > @@ -339,6 +344,10 @@ static struct vport *gre_create(const struct > vport_parms *parms) > strncpy(vport_priv(vport), parms->name, IFNAMSIZ); > rcu_assign_pointer(ovs_net->vport_net.gre_vport, vport); > return vport; > + > +error: > + gre_exit(); > + return vport; > } > > static void gre_tnl_destroy(struct vport *vport) > @@ -350,6 +359,7 @@ static void gre_tnl_destroy(struct vport *vport) > > rcu_assign_pointer(ovs_net->vport_net.gre_vport, NULL); > ovs_vport_deferred_free(vport); > + gre_exit(); > } > > static int gre_tnl_send(struct vport *vport, struct sk_buff *skb) > @@ -366,8 +376,6 @@ static int gre_tnl_send(struct vport *vport, struct > sk_buff *skb) > const struct vport_ops ovs_gre_vport_ops = { > .type = OVS_VPORT_TYPE_GRE, > .flags = VPORT_F_TUN_ID, > - .init = gre_init, > - .exit = gre_exit, > .create = gre_create, > .destroy = gre_tnl_destroy, > .get_name = gre_get_name, > @@ -380,18 +388,28 @@ static struct vport *gre64_create(const struct > vport_parms *parms) > struct net *net = ovs_dp_get_net(parms->dp); > struct ovs_net *ovs_net; > struct vport *vport; > + int err; > + > + err = gre_init(); > + if (err) > + return ERR_PTR(err); > > ovs_net = net_generic(net, ovs_net_id); > - if (ovsl_dereference(ovs_net->vport_net.gre64_vport)) > - return ERR_PTR(-EEXIST); > + if (ovsl_dereference(ovs_net->vport_net.gre64_vport)) { > + vport = ERR_PTR(-EEXIST); > + goto error; > + } > > vport = ovs_vport_alloc(IFNAMSIZ, &ovs_gre64_vport_ops, parms); > if (IS_ERR(vport)) > - return vport; > + goto error; > > strncpy(vport_priv(vport), parms->name, IFNAMSIZ); > rcu_assign_pointer(ovs_net->vport_net.gre64_vport, vport); > return vport; > +error: > + gre_exit(); > + return vport; > } > > static void gre64_tnl_destroy(struct vport *vport) > @@ -403,6 +421,7 @@ static void gre64_tnl_destroy(struct vport *vport) > > rcu_assign_pointer(ovs_net->vport_net.gre64_vport, NULL); > ovs_vport_deferred_free(vport); > + gre_exit(); > } > > static int gre64_tnl_send(struct vport *vport, struct sk_buff *skb) > @@ -419,8 +438,6 @@ static int gre64_tnl_send(struct vport *vport, struct > sk_buff *skb) > const struct vport_ops ovs_gre64_vport_ops = { > .type = OVS_VPORT_TYPE_GRE64, > .flags = VPORT_F_TUN_ID, > - .init = gre_init, > - .exit = gre_exit, > .create = gre64_create, > .destroy = gre64_tnl_destroy, > .get_name = gre_get_name, > diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c > index 1e8c304..1b19bfc 100644 > --- a/datapath/vport-internal_dev.c > +++ b/datapath/vport-internal_dev.c > @@ -294,7 +294,6 @@ static int internal_dev_recv(struct vport *vport, > struct sk_buff *skb) > > const struct vport_ops ovs_internal_vport_ops = { > .type = OVS_VPORT_TYPE_INTERNAL, > - .flags = VPORT_F_REQUIRED, > .create = internal_dev_create, > .destroy = internal_dev_destroy, > .get_name = ovs_netdev_get_name, > diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c > index 4558079..434ab4d 100644 > --- a/datapath/vport-netdev.c > +++ b/datapath/vport-netdev.c > @@ -117,17 +117,27 @@ static int netdev_frame_hook(struct net_bridge_port > *p, struct sk_buff **pskb) > static int netdev_init(void) { return 0; } > static void netdev_exit(void) { } > #else > -static int netdev_init(void) > +static int port_count; > + > +static void netdev_init(void) > { > + port_count++; > + if (port_count > 1) > + return; > + > /* Hook into callback used by the bridge to intercept packets. > * Parasites we are. */ > br_handle_frame_hook = netdev_frame_hook; > > - return 0; > + return; > } > > static void netdev_exit(void) > { > + port_count--; > + if (port_count > 0) > + return; > + > br_handle_frame_hook = NULL; > } > #endif > @@ -179,6 +189,7 @@ static struct vport *netdev_create(const struct > vport_parms *parms) > netdev_vport->dev->priv_flags |= IFF_OVS_DATAPATH; > rtnl_unlock(); > > + netdev_init(); > return vport; > > #ifndef HAVE_RHEL_OVS_HOOK > @@ -212,6 +223,7 @@ static void netdev_destroy(struct vport *vport) > { > struct netdev_vport *netdev_vport = netdev_vport_priv(vport); > > + netdev_exit(); > rtnl_lock(); > netdev_vport->dev->priv_flags &= ~IFF_OVS_DATAPATH; > netdev_rx_handler_unregister(netdev_vport->dev); > @@ -388,9 +400,6 @@ struct vport *ovs_netdev_get_vport(struct net_device > *dev) > > const struct vport_ops ovs_netdev_vport_ops = { > .type = OVS_VPORT_TYPE_NETDEV, > - .flags = VPORT_F_REQUIRED, > - .init = netdev_init, > - .exit = netdev_exit, > .create = netdev_create, > .destroy = netdev_destroy, > .get_name = ovs_netdev_get_name, > diff --git a/datapath/vport.c b/datapath/vport.c > index b63ed59..2f007ae 100644 > --- a/datapath/vport.c > +++ b/datapath/vport.c > @@ -36,7 +36,7 @@ > > /* List of statically compiled vport implementations. Don't forget to > also > * add yours to the list at the bottom of vport.h. */ > -static const struct vport_ops *base_vport_ops_list[] = { > +static const struct vport_ops *vport_ops_list[] = { > &ovs_netdev_vport_ops, > &ovs_internal_vport_ops, > &ovs_gre_vport_ops, > @@ -47,9 +47,6 @@ static const struct vport_ops *base_vport_ops_list[] = { > #endif > }; > > -static const struct vport_ops **vport_ops_list; > -static int n_vport_types; > - > /* Protected by RCU read lock for reading, ovs_mutex for writing. */ > static struct hlist_head *dev_table; > #define VPORT_HASH_BUCKETS 1024 > @@ -57,68 +54,25 @@ static struct hlist_head *dev_table; > /** > * ovs_vport_init - initialize vport subsystem > * > - * Called at module load time to initialize the vport subsystem and any > - * compiled in vport types. > + * Called at module load time to initialize the vport subsystem. > */ > int ovs_vport_init(void) > { > - int err; > - int i; > - > dev_table = kzalloc(VPORT_HASH_BUCKETS * sizeof(struct hlist_head), > GFP_KERNEL); > - if (!dev_table) { > - err = -ENOMEM; > - goto error; > - } > - > - vport_ops_list = kmalloc(ARRAY_SIZE(base_vport_ops_list) * > - sizeof(struct vport_ops *), GFP_KERNEL); > - if (!vport_ops_list) { > - err = -ENOMEM; > - goto error_dev_table; > - } > - > - for (i = 0; i < ARRAY_SIZE(base_vport_ops_list); i++) { > - const struct vport_ops *new_ops = base_vport_ops_list[i]; > - > - if (new_ops->init) > - err = new_ops->init(); > - else > - err = 0; > - > - if (!err) > - vport_ops_list[n_vport_types++] = new_ops; > - else if (new_ops->flags & VPORT_F_REQUIRED) { > - ovs_vport_exit(); > - goto error; > - } > - } > + if (!dev_table) > + return -ENOMEM; > > return 0; > - > -error_dev_table: > - kfree(dev_table); > -error: > - return err; > } > > /** > * ovs_vport_exit - shutdown vport subsystem > * > - * Called at module exit time to shutdown the vport subsystem and any > - * initialized vport types. > + * Called at module exit time to shutdown the vport subsystem. > */ > void ovs_vport_exit(void) > { > - int i; > - > - for (i = 0; i < n_vport_types; i++) { > - if (vport_ops_list[i]->exit) > - vport_ops_list[i]->exit(); > - } > - > - kfree(vport_ops_list); > kfree(dev_table); > } > > @@ -222,7 +176,7 @@ struct vport *ovs_vport_add(const struct vport_parms > *parms) > int err = 0; > int i; > > - for (i = 0; i < n_vport_types; i++) { > + for (i = 0; i < ARRAY_SIZE(vport_ops_list); i++) { > if (vport_ops_list[i]->type == parms->type) { > struct hlist_head *bucket; > > diff --git a/datapath/vport.h b/datapath/vport.h > index cba578c..56065a7 100644 > --- a/datapath/vport.h > +++ b/datapath/vport.h > @@ -94,8 +94,7 @@ struct vport { > struct ovs_vport_stats offset_stats; > }; > > -#define VPORT_F_REQUIRED (1 << 0) /* If init fails, module loading > fails. */ > -#define VPORT_F_TUN_ID (1 << 1) /* Sets OVS_CB(skb)->tun_id. */ > +#define VPORT_F_TUN_ID (1 << 0) /* Sets OVS_CB(skb)->tun_id. */ > > /** > * struct vport_parms - parameters for creating a new vport > @@ -124,10 +123,6 @@ struct vport_parms { > * @type: %OVS_VPORT_TYPE_* value for this type of virtual port. > * @flags: Flags of type VPORT_F_* that influence how the generic vport > layer > * handles this vport. > - * @init: Called at module initialization. If VPORT_F_REQUIRED is set > then the > - * failure of this function will cause the module to not load. If the > flag is > - * not set and initialzation fails then no vports of this type can be > created. > - * @exit: Called at module unload. > * @create: Create a new vport configured as specified. On success > returns > * a new vport allocated with ovs_vport_alloc(), otherwise an ERR_PTR() > value. > * @destroy: Destroys a vport. Must call vport_free() on the vport but > not > @@ -146,10 +141,6 @@ struct vport_ops { > enum ovs_vport_type type; > u32 flags; > > - /* Called at module init and exit respectively. */ > - int (*init)(void); > - void (*exit)(void); > - > /* Called with ovs_mutex. */ > struct vport *(*create)(const struct vport_parms *); > void (*destroy)(struct vport *); > -- > 1.7.1 > > > > ------------------------------ > > Message: 2 > Date: Tue, 30 Apr 2013 17:11:17 -0700 > From: Jesse Gross <je...@nicira.com> > Subject: Re: [ovs-dev] [PATCH v2 1/2] tunneling: Remove struct > tnl_vport and tnl_ops. > To: Pravin B Shelar <pshe...@nicira.com> > Cc: "dev@openvswitch.org" <dev@openvswitch.org> > Message-ID: > <CAEP_g= > 8fxsp_g3hpektaskhq9qcxltex-feemfdcjrqes63...@mail.gmail.com> > Content-Type: text/plain; charset=UTF-8 > > On Tue, Apr 30, 2013 at 4:20 PM, Pravin B Shelar <pshe...@nicira.com> > wrote: > > diff --git a/datapath/tunnel.c b/datapath/tunnel.c > > index 057aaed..fb430f2 100644 > > --- a/datapath/tunnel.c > > +++ b/datapath/tunnel.c > > @@ -234,17 +221,32 @@ int ovs_tnl_send(struct vport *vport, struct > sk_buff *skb) > > rt = find_route(ovs_dp_get_net(vport->dp), > > &saddr, > > OVS_CB(skb)->tun_key->ipv4_dst, > > - tnl_vport->tnl_ops->ipproto, > > + ipproto, > > OVS_CB(skb)->tun_key->ipv4_tos, > > skb_get_mark(skb)); > > if (IS_ERR(rt)) > > goto error_free; > > > > - /* Offloading */ > > - tunnel_hlen = tnl_vport->tnl_ops->hdr_len(OVS_CB(skb)->tun_key); > > tunnel_hlen += sizeof(struct iphdr); > > > > - skb = handle_offloads(skb, rt, tunnel_hlen); > > + min_headroom = LL_RESERVED_SPACE(rt_dst(rt).dev) + > rt_dst(rt).header_len > > + + tunnel_hlen > > + + (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0); > > + > > + if (skb_headroom(skb) < min_headroom || skb_header_cloned(skb)) { > > + int err; > > + int head_delta = SKB_DATA_ALIGN(min_headroom - > > + skb_headroom(skb) + > > + 16); > > + > > + err = pskb_expand_head(skb, max_t(int, head_delta, 0), > > + 0, GFP_ATOMIC); > > + if (unlikely(err)) > > + goto error_free; > > + } > > + > > + /* Offloading */ > > + skb = handle_offloads(skb, rt); > > I'm not sure I understand why the code block that expands the headroom > was moved out of handle_offloads(). However, I see a couple of issues: > * It leaks the rt in the case of an error. > * rt is not used in handle_offloads(). > > > diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c > > index 1850fc2..92b1666 100644 > > --- a/datapath/vport-vxlan.c > > +++ b/datapath/vport-vxlan.c > > /** > > * struct vxlan_port - Keeps track of open UDP ports > > - * @list: list element. > > - * @vport: vport for the tunnel. > > + * @dst_port: vxlan UDP port no. > > + * @list: list element in @vxlan_ports. > > * @socket: The socket created for this port number. > > + * @name: vport name. > > + * @rcu: RCU callback head for deferred destruction. > > */ > > struct vxlan_port { > > + __be16 dst_port; > > struct list_head list; > > - struct vport *vport; > > struct socket *vxlan_rcv_socket; > > + char name[IFNAMSIZ]; > > struct rcu_head rcu; > > We're not using the 'rcu' element anymore, are we? > > > ------------------------------ > > Message: 3 > Date: Tue, 30 Apr 2013 17:38:46 -0700 > From: Jesse Gross <je...@nicira.com> > Subject: Re: [ovs-dev] [PATCH v2 2/2] datapath: Move vport init to > First port create. > To: Pravin B Shelar <pshe...@nicira.com> > Cc: "dev@openvswitch.org" <dev@openvswitch.org> > Message-ID: > <CAEP_g=- > 5y8xl7z48czfz6ukrlmua9ocbyffo6rv8urqama0...@mail.gmail.com> > Content-Type: text/plain; charset=UTF-8 > > On Tue, Apr 30, 2013 at 4:21 PM, Pravin B Shelar <pshe...@nicira.com> > wrote: > > diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c > > index 2db4934..59e22be 100644 > > --- a/datapath/vport-gre.c > > +++ b/datapath/vport-gre.c > > @@ -327,10 +325,17 @@ static struct vport *gre_create(const struct > vport_parms *parms) > > struct net *net = ovs_dp_get_net(parms->dp); > > struct ovs_net *ovs_net; > > struct vport *vport; > > + int err; > > + > > + err = gre_init(); > > + if (err) > > + return ERR_PTR(err); > > > > ovs_net = net_generic(net, ovs_net_id); > > - if (ovsl_dereference(ovs_net->vport_net.gre_vport)) > > - return ERR_PTR(-EEXIST); > > + if (ovsl_dereference(ovs_net->vport_net.gre_vport)) { > > + vport = ERR_PTR(-EEXIST); > > + goto error; > > + } > > I think that we need to jump to error in the case that > ovs_vport_alloc() fails as well. > > > diff --git a/datapath/vport.h b/datapath/vport.h > > index cba578c..56065a7 100644 > > --- a/datapath/vport.h > > +++ b/datapath/vport.h > > @@ -94,8 +94,7 @@ struct vport { > > struct ovs_vport_stats offset_stats; > > }; > > > > -#define VPORT_F_REQUIRED (1 << 0) /* If init fails, module > loading fails. */ > > -#define VPORT_F_TUN_ID (1 << 1) /* Sets OVS_CB(skb)->tun_id. */ > > +#define VPORT_F_TUN_ID (1 << 0) /* Sets OVS_CB(skb)->tun_id. */ > > This is separate but we should probably rename TUN_ID now that it's > more than just the ID. Maybe we can even just remove it (for example, > force people to initialize it) now that it's the only remaining flag. > > > ------------------------------ > > Message: 4 > Date: Tue, 30 Apr 2013 17:49:24 -0700 > From: Pravin Shelar <pshe...@nicira.com> > Subject: Re: [ovs-dev] [PATCH v2 1/2] tunneling: Remove struct > tnl_vport and tnl_ops. > To: Jesse Gross <je...@nicira.com> > Cc: "dev@openvswitch.org" <dev@openvswitch.org> > Message-ID: > <CALnjE+pjpwLGZRm0pt159Kvai=23F7VN8gaQPeV2i= > rrfg3...@mail.gmail.com> > Content-Type: text/plain; charset=ISO-8859-1 > > On Tue, Apr 30, 2013 at 5:11 PM, Jesse Gross <je...@nicira.com> wrote: > > On Tue, Apr 30, 2013 at 4:20 PM, Pravin B Shelar <pshe...@nicira.com> > wrote: > >> diff --git a/datapath/tunnel.c b/datapath/tunnel.c > >> index 057aaed..fb430f2 100644 > >> --- a/datapath/tunnel.c > >> +++ b/datapath/tunnel.c > >> @@ -234,17 +221,32 @@ int ovs_tnl_send(struct vport *vport, struct > sk_buff *skb) > >> rt = find_route(ovs_dp_get_net(vport->dp), > >> &saddr, > >> OVS_CB(skb)->tun_key->ipv4_dst, > >> - tnl_vport->tnl_ops->ipproto, > >> + ipproto, > >> OVS_CB(skb)->tun_key->ipv4_tos, > >> skb_get_mark(skb)); > >> if (IS_ERR(rt)) > >> goto error_free; > >> > >> - /* Offloading */ > >> - tunnel_hlen = tnl_vport->tnl_ops->hdr_len(OVS_CB(skb)->tun_key); > >> tunnel_hlen += sizeof(struct iphdr); > >> > >> - skb = handle_offloads(skb, rt, tunnel_hlen); > >> + min_headroom = LL_RESERVED_SPACE(rt_dst(rt).dev) + > rt_dst(rt).header_len > >> + + tunnel_hlen > >> + + (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0); > >> + > >> + if (skb_headroom(skb) < min_headroom || skb_header_cloned(skb)) > { > >> + int err; > >> + int head_delta = SKB_DATA_ALIGN(min_headroom - > >> + skb_headroom(skb) + > >> + 16); > >> + > >> + err = pskb_expand_head(skb, max_t(int, head_delta, 0), > >> + 0, GFP_ATOMIC); > >> + if (unlikely(err)) > >> + goto error_free; > >> + } > >> + > >> + /* Offloading */ > >> + skb = handle_offloads(skb, rt); > > > > I'm not sure I understand why the code block that expands the headroom > > was moved out of handle_offloads(). However, I see a couple of issues: > > * It leaks the rt in the case of an error. > > * rt is not used in handle_offloads(). > > > > I wanted to keep only offloading bits in handle offload, Thats why I > moved headroom related code out. I will fix both issues. > > >> diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c > >> index 1850fc2..92b1666 100644 > >> --- a/datapath/vport-vxlan.c > >> +++ b/datapath/vport-vxlan.c > >> /** > >> * struct vxlan_port - Keeps track of open UDP ports > >> - * @list: list element. > >> - * @vport: vport for the tunnel. > >> + * @dst_port: vxlan UDP port no. > >> + * @list: list element in @vxlan_ports. > >> * @socket: The socket created for this port number. > >> + * @name: vport name. > >> + * @rcu: RCU callback head for deferred destruction. > >> */ > >> struct vxlan_port { > >> + __be16 dst_port; > >> struct list_head list; > >> - struct vport *vport; > >> struct socket *vxlan_rcv_socket; > >> + char name[IFNAMSIZ]; > >> struct rcu_head rcu; > > > > We're not using the 'rcu' element anymore, are we? > > right, It is not required. > > > ------------------------------ > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > > > End of dev Digest, Vol 45, Issue 173 > ************************************ >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
