[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