[ovs-dev,net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
diff mbox series

Message ID 20190705160809.5202-1-ap420073@gmail.com
State New
Headers show
Series
  • [ovs-dev,net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
Related show

Commit Message

Taehee Yoo July 5, 2019, 4:08 p.m. UTC
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(-)

Comments

David Miller July 8, 2019, 11:08 p.m. UTC | #1
From: Taehee Yoo <ap420073@gmail.com>
Date: Sat,  6 Jul 2019 01:08:09 +0900

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

I'm not so sure about the logic here and I'd therefore like an OVS expert
to review this.

Thanks.
Gregory Rose July 8, 2019, 11:18 p.m. UTC | #2
On 7/8/2019 4:08 PM, David Miller wrote:
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Sat,  6 Jul 2019 01:08:09 +0900
>
>> 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>
> I'm not so sure about the logic here and I'd therefore like an OVS expert
> to review this.

I'll review and test it and get back.  Pravin may have input as well.

Thanks,

- Greg

> Thanks.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Gregory Rose July 8, 2019, 11:22 p.m. UTC | #3
On 7/8/2019 4:18 PM, Gregory Rose wrote:
> On 7/8/2019 4:08 PM, David Miller wrote:
>> From: Taehee Yoo <ap420073@gmail.com>
>> Date: Sat,  6 Jul 2019 01:08:09 +0900
>>
>>> 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>
>> I'm not so sure about the logic here and I'd therefore like an OVS 
>> expert
>> to review this.
>
> I'll review and test it and get back.  Pravin may have input as well.
>

Err, adding Pravin.

- Greg

> Thanks,
>
> - Greg
>
>> Thanks.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Pravin Shelar July 11, 2019, 9:07 p.m. UTC | #4
I was bit busy for last couple of days. I will finish review by EOD today.

Thanks,
Pravin.

On Mon, Jul 8, 2019 at 4:22 PM Gregory Rose <gvrose8192@gmail.com> wrote:
>
>
>
> On 7/8/2019 4:18 PM, Gregory Rose wrote:
> > On 7/8/2019 4:08 PM, David Miller wrote:
> >> From: Taehee Yoo <ap420073@gmail.com>
> >> Date: Sat,  6 Jul 2019 01:08:09 +0900
> >>
> >>> 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>
> >> I'm not so sure about the logic here and I'd therefore like an OVS
> >> expert
> >> to review this.
> >
> > I'll review and test it and get back.  Pravin may have input as well.
> >
>
> Err, adding Pravin.
>
> - Greg
>
> > Thanks,
> >
> > - Greg
> >
> >> Thanks.
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
Gregory Rose July 11, 2019, 11:55 p.m. UTC | #5
On 7/11/2019 2:07 PM, Pravin Shelar wrote:
> I was bit busy for last couple of days. I will finish review by EOD today.
>
> Thanks,
> Pravin.

net-next is closed anyway so no rush, but thanks!

- Greg

>
> On Mon, Jul 8, 2019 at 4:22 PM Gregory Rose <gvrose8192@gmail.com> wrote:
>>
>>
>> On 7/8/2019 4:18 PM, Gregory Rose wrote:
>>> On 7/8/2019 4:08 PM, David Miller wrote:
>>>> From: Taehee Yoo <ap420073@gmail.com>
>>>> Date: Sat,  6 Jul 2019 01:08:09 +0900
>>>>
>>>>> 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>
>>>> I'm not so sure about the logic here and I'd therefore like an OVS
>>>> expert
>>>> to review this.
>>> I'll review and test it and get back.  Pravin may have input as well.
>>>
>> Err, adding Pravin.
>>
>> - Greg
>>
>>> Thanks,
>>>
>>> - Greg
>>>
>>>> Thanks.
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Gregory Rose July 12, 2019, 4:18 p.m. UTC | #6
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>
David Miller July 12, 2019, 10:18 p.m. UTC | #7
From: Taehee Yoo <ap420073@gmail.com>
Date: Sat,  6 Jul 2019 01:08:09 +0900

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

I don't think Taehee should be punished because it took several days
to get someone to look at and review and/or test this patch and
meanwhile the net-next tree closed down.

I ask for maintainer review as both a courtesy and a way to lessen
my workload.  But if that means patches rot for days in patchwork
I'm just going to apply them after my own review.

So I'm applying this now.
Gregory Rose July 12, 2019, 11:11 p.m. UTC | #8
On 7/12/2019 3:18 PM, David Miller wrote:
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Sat,  6 Jul 2019 01:08:09 +0900
>
>> 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>
> I don't think Taehee should be punished because it took several days
> to get someone to look at and review and/or test this patch and
> meanwhile the net-next tree closed down.
>
> I ask for maintainer review as both a courtesy and a way to lessen
> my workload.  But if that means patches rot for days in patchwork
> I'm just going to apply them after my own review.
>
> So I'm applying this now.
>
My apologies Dave.  I did test and review the patch, perhaps you didn't 
see it.  In any case, you're right, Taehee was owed a more timely review 
and I missed it.

Thanks for applying the patch.

- Greg
Pravin Shelar July 15, 2019, 6:15 p.m. UTC | #9
On Fri, Jul 5, 2019 at 9:08 AM Taehee Yoo <ap420073@gmail.com> 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>

Sorry for late reply. This patch looks good to me too.

I am curious about reason to avoid adjustment to headroom. why are you
trying to avoid unnecessary changes to headroom.

Thanks,
Pravin.
Taehee Yoo July 16, 2019, 12:28 p.m. UTC | #10
On Tue, 16 Jul 2019 at 03:15, Pravin Shelar <pshelar@ovn.org> wrote:
>

Hi Pravin,

> On Fri, Jul 5, 2019 at 9:08 AM Taehee Yoo <ap420073@gmail.com> 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>
>
> Sorry for late reply. This patch looks good to me too.
>
> I am curious about reason to avoid adjustment to headroom. why are you
> trying to avoid unnecessary changes to headroom.
>

Thank you for the review!
The intention of this patch is to remove unnecessary overhead.
There is no other reason.

Thanks!

> Thanks,
> Pravin.

Patch
diff mbox series

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);