Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom

[Gregory Rose <gvrose8192@gmail.com>]

On 7/5/2019 9:08 AM, Taehee Yoo wrote:
> When a vport is deleted, the maximum headroom size would be changed.
> If the vport which has the largest headroom is deleted,
> the new max_headroom would be set.
> But, if the new headroom size is equal to the old headroom size,
> updating routine is unnecessary.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
>   net/openvswitch/datapath.c | 39 +++++++++++++++++++++++++++-----------
>   1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 33b388103741..892287d06c17 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1958,10 +1958,9 @@ static struct vport *lookup_vport(struct net *net,
>   
>   }
>   
> -/* Called with ovs_mutex */
> -static void update_headroom(struct datapath *dp)
> +static unsigned int ovs_get_max_headroom(struct datapath *dp)
>   {
> -	unsigned dev_headroom, max_headroom = 0;
> +	unsigned int dev_headroom, max_headroom = 0;
>   	struct net_device *dev;
>   	struct vport *vport;
>   	int i;
> @@ -1975,10 +1974,19 @@ static void update_headroom(struct datapath *dp)
>   		}
>   	}
>   
> -	dp->max_headroom = max_headroom;
> +	return max_headroom;
> +}
> +
> +/* Called with ovs_mutex */
> +static void ovs_update_headroom(struct datapath *dp, unsigned int new_headroom)
> +{
> +	struct vport *vport;
> +	int i;
> +
> +	dp->max_headroom = new_headroom;
>   	for (i = 0; i < DP_VPORT_HASH_BUCKETS; i++)
>   		hlist_for_each_entry_rcu(vport, &dp->ports[i], dp_hash_node)
> -			netdev_set_rx_headroom(vport->dev, max_headroom);
> +			netdev_set_rx_headroom(vport->dev, new_headroom);
>   }
>   
>   static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
> @@ -1989,6 +1997,7 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
>   	struct sk_buff *reply;
>   	struct vport *vport;
>   	struct datapath *dp;
> +	unsigned int new_headroom;
>   	u32 port_no;
>   	int err;
>   
> @@ -2050,8 +2059,10 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
>   				      info->snd_portid, info->snd_seq, 0,
>   				      OVS_VPORT_CMD_NEW);
>   
> -	if (netdev_get_fwd_headroom(vport->dev) > dp->max_headroom)
> -		update_headroom(dp);
> +	new_headroom = netdev_get_fwd_headroom(vport->dev);
> +
> +	if (new_headroom > dp->max_headroom)
> +		ovs_update_headroom(dp, new_headroom);
>   	else
>   		netdev_set_rx_headroom(vport->dev, dp->max_headroom);
>   
> @@ -2122,11 +2133,12 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
>   
>   static int ovs_vport_cmd_del(struct sk_buff *skb, struct genl_info *info)
>   {
> -	bool must_update_headroom = false;
> +	bool update_headroom = false;
>   	struct nlattr **a = info->attrs;
>   	struct sk_buff *reply;
>   	struct datapath *dp;
>   	struct vport *vport;
> +	unsigned int new_headroom;
>   	int err;
>   
>   	reply = ovs_vport_cmd_alloc_info();
> @@ -2152,12 +2164,17 @@ static int ovs_vport_cmd_del(struct sk_buff *skb, struct genl_info *info)
>   	/* the vport deletion may trigger dp headroom update */
>   	dp = vport->dp;
>   	if (netdev_get_fwd_headroom(vport->dev) == dp->max_headroom)
> -		must_update_headroom = true;
> +		update_headroom = true;
> +
>   	netdev_reset_rx_headroom(vport->dev);
>   	ovs_dp_detach_port(vport);
>   
> -	if (must_update_headroom)
> -		update_headroom(dp);
> +	if (update_headroom) {
> +		new_headroom = ovs_get_max_headroom(dp);
> +
> +		if (new_headroom < dp->max_headroom)
> +			ovs_update_headroom(dp, new_headroom);
> +	}
>   	ovs_unlock();
>   
>   	ovs_notify(&dp_vport_genl_family, reply, info);

I did a compile test and ran the OVS kernel self-test suite.  Looks OK 
to me.
Tested-by: Greg Rose <gvrose8192@gmail.com>
Reviewed-by: Greg Rose <gvrose8192@gmail.com>