[ovs-dev] [PATCH 2/2] datapath: Removal of kernel compatibility code for upstreaming
Jesse Gross
jesse at nicira.com
Thu Nov 10 19:30:32 UTC 2011
On Wed, Nov 9, 2011 at 6:40 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> Following patch deletes OVS compatibility code related to older kernel,
> bridge, vlan etc and rearranges it for upstreaming.
>
> This patch is against OVS upstream kernel tree.
>
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> Bug #7561
This one is less critical because it will eventually get squashed into
the main OVS commit but can you still follow the same rules as before
for the commit message? Also the comment about which tree the patch
is against doesn't really belong in the commit message itself. It
should go in the email header i.e [PATCH upstream], a comment before
or after the commit message or be omitted entirely because the file
paths make it unambiguous what tree it targets.
Overall, I think this is a good first pass. My goals for the cleanup are:
1. Make the tree compile.
2. Remove compatibility code.
3. Smooth things out so it doesn't look like we just deleted a bunch
of compatibility code.
I think this patch does a pretty good job of #1 and #2 (although there
is a still a little more to be done there as well) and we'll want to
do another pass to handle #3. That doesn't all need to be done in a
single patch though so I'll apply patches that move in the right
direction even if they aren't complete.
Of the things that I comment on below the only ones that would
actually prevent me from accepting this patch are:
* Memory leaks
* Changes made here that really belong in the OVS tree (i.e. the
checksum changes are important, the vlan ones are not).
* Unnecessary/incorrect changes.
> diff --git a/net/openvswitch/actions.h b/net/openvswitch/actions.h
> index a832295..92e51bd 100644
> --- a/net/openvswitch/actions.h
> +++ b/net/openvswitch/actions.h
> @@ -13,16 +13,13 @@
> #include <linux/version.h>
>
> struct datapath;
> -struct sk_buff;
> struct sw_flow_key;
>
> int execute_actions(struct datapath *dp, struct sk_buff *skb);
>
> static inline void skb_clear_rxhash(struct sk_buff *skb)
> {
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35)
> skb->rxhash = 0;
> -#endif
> }
I think that we probably want to just put this inline in a future patch.
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 5660f2d..a35872b 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -259,7 +152,6 @@ static struct vport *new_vport(const struct vport_parms *parms)
> rcu_assign_pointer(dp->ports[parms->port_no], vport);
> list_add(&vport->node, &dp->port_list);
>
> - dp_ifinfo_notify(RTM_NEWLINK, vport);
There's an extra blank line now.
> +static inline int vlan_deaccel_tag(struct sk_buff *skb)
> +{
> + if (!vlan_tx_tag_present(skb))
> + return 0;
> +
> + skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
> + if (unlikely(!skb))
> + return -ENOMEM;
> +
> + skb->vlan_tci = 0;
> + return 0;
> +}
I think in the future we should probably write this inline.
> @@ -481,10 +365,8 @@ static int queue_userspace_packet(int dp_ifindex, struct sk_buff *skb,
> nla_get_u64(upcall_info->userdata));
>
> nla = __nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, skb->len);
> - if (skb->ip_summed == CHECKSUM_PARTIAL)
> - copy_and_csum_skb(skb, nla_data(nla));
> - else
> - skb_copy_bits(skb, 0, nla_data(nla), skb->len);
> +
> + skb_copy_and_csum_dev(skb, nla_data(nla));
We should make this change in the OVS repository and pull it over
instead of just doing it upstream.
> @@ -1790,24 +1605,17 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
[...]
> + if (IS_ERR(reply)) {
> + err = PTR_ERR(reply);
> dp_detach_port(vport);
> goto exit_unlock;
> }
> +
> genl_notify(reply, genl_info_net(info), info->snd_pid,
> dp_vport_multicast_group.id, info->nlhdr, GFP_KERNEL);
>
> -
> exit_unlock:
Please don't add unnecessary whitespace changes to this tree.
> @@ -2057,16 +1848,11 @@ static int __init dp_init(void)
>
> BUILD_BUG_ON(sizeof(struct ovs_skb_cb) > sizeof(dummy_skb->cb));
>
> - pr_info("Open vSwitch %s, built "__DATE__" "__TIME__"\n",
> - VERSION BUILDNR);
> -
> - err = tnl_init();
> - if (err)
> - goto error;
> + pr_info("Open vSwitch built "__DATE__" "__TIME__"\n");
It's not usual for an in-tree kernel module to print its build time.
Maybe something simply descriptive like "Open vSwitch forwarding
datapath".
> err = flow_init();
> if (err)
> - goto error_tnl_exit;
> + return err;
Please try to keep structural symmetry as much as possible in this
tree (if you want to change it then do so in the OVS tree).
There's now a memory leak when deleting datapaths. Previously the dp
was freed when the kobj reference count dropped to zero but that code
has been removed.
> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index 43360cc..9807e60 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
There's a few needless whitespace changes in this file.
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index 26b432b..ec58c4c 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> +static void vport_gen_rand_ether_addr(u8 *addr)
> +{
> + random_ether_addr(addr);
> +
> + /* Set the OUI to the Nicira one. */
> + addr[0] = 0x00;
> + addr[1] = 0x23;
> + addr[2] = 0x20;
> +
> + /* Set the top bit to indicate random address. */
> + addr[3] |= 0x80;
> +}
I think we should probably stop setting the OUI to Nicira's in the OVS
tree and just use a random address.
> const struct vport_ops internal_vport_ops = {
> .type = OVS_VPORT_TYPE_INTERNAL,
> - .flags = VPORT_F_REQUIRED | VPORT_F_FLOW,
> + .flags = VPORT_F_FLOW,
VPORT_F_FLOW relates only to tunneling, so it is never used here. We
should remove this as well as all the refcounting infrastructure in
flow.c and the usage in datapath.c
> -int is_internal_vport(const struct vport *vport)
> -{
> - return vport->ops == &internal_vport_ops;
> }
Apparently nothing uses this at all, so we should remove it from the
OVS tree (and from the header file).
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index a6b686c..dd0c128 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> -static void release_vport(struct kobject *kobj)
> -{
> - struct vport *p = container_of(kobj, struct vport, kobj);
> - kfree(p);
> -}
Removing this introduces a memory leak here as well.
> -/**
> - * vport_set_stats - sets offset device stats
> - *
> - * @vport: vport on which to set stats
> - * @stats: stats to set
> - *
> - * Provides a set of transmit, receive, and error stats to be added as an
> - * offset to the collect data when stats are retreived. Some devices may not
> - * support setting the stats, in which case the result will always be
> - * -EOPNOTSUPP.
> - *
> - * Must be called with RTNL lock.
> - */
> -void vport_set_stats(struct vport *vport, struct ovs_vport_stats *stats)
> -{
> - ASSERT_RTNL();
> -
> - spin_lock_bh(&vport->stats_lock);
> - vport->offset_stats = *stats;
> - spin_unlock_bh(&vport->stats_lock);
> -}
We should completely remove offset_stats now that they are no longer used.
> diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
> index 3dbc68f..dac5c51 100644
> --- a/net/openvswitch/vport.h
> +++ b/net/openvswitch/vport.h
> @@ -30,8 +30,6 @@ void vport_del(struct vport *);
>
> struct vport *vport_locate(const char *name);
>
> -int vport_set_addr(struct vport *, const unsigned char *);
> -void vport_set_stats(struct vport *, struct ovs_vport_stats *);
> void vport_get_stats(struct vport *, struct ovs_vport_stats *);
>
> int vport_set_options(struct vport *, struct nlattr *options);
> @@ -61,7 +59,6 @@ struct vport_err_stats {
> * @rcu: RCU callback head for deferred destruction.
> * @port_no: Index into @dp's @ports array.
> * @dp: Datapath to which this port belongs.
> - * @kobj: Represents /sys/class/net/<devname>/brport.
> * @linkname: The name of the link from /sys/class/net/<datapath>/brif to this
linkname should go away as well.
More information about the dev
mailing list