[ovs-dev] [PATCH 2/2] datapath: Removal of kernel compatibility code for upstreaming

Pravin Shelar pshelar at nicira.com
Thu Nov 10 23:40:29 UTC 2011


On Thu, Nov 10, 2011 at 11:30 AM, Jesse Gross <jesse at nicira.com> wrote:
> 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.
OK

>
>> 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.
OK, i will keep it as it is.

>
>> @@ -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.
I have posted patch for same.

>
>> @@ -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".
ok.

>
>>        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).
ok.
>
> 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.
>
right, I need to kfree it.

>> 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.
ok.
>
>> 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.
>
I have posted patch for this.

>>  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
OK.

>
>> -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).
>
posted patch for same.

>> 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.
right.

>
>> -/**
>> - *     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.
OK.

>
>> 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.
>
OK.



More information about the dev mailing list