* [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
@ 2019-07-05 16:08 Taehee Yoo
2019-07-08 23:08 ` David Miller
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Taehee Yoo @ 2019-07-05 16:08 UTC (permalink / raw)
To: davem, pshelar, netdev, dev; +Cc: ap420073
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-05 16:08 [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom Taehee Yoo
@ 2019-07-08 23:08 ` David Miller
2019-07-08 23:18 ` [ovs-dev] " Gregory Rose
2019-07-12 16:18 ` Gregory Rose
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2019-07-08 23:08 UTC (permalink / raw)
To: ap420073; +Cc: pshelar, netdev, dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-08 23:08 ` David Miller
@ 2019-07-08 23:18 ` Gregory Rose
2019-07-08 23:22 ` Gregory Rose
0 siblings, 1 reply; 11+ messages in thread
From: Gregory Rose @ 2019-07-08 23:18 UTC (permalink / raw)
To: David Miller, ap420073; +Cc: dev, netdev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-08 23:18 ` [ovs-dev] " Gregory Rose
@ 2019-07-08 23:22 ` Gregory Rose
2019-07-11 21:07 ` Pravin Shelar
0 siblings, 1 reply; 11+ messages in thread
From: Gregory Rose @ 2019-07-08 23:22 UTC (permalink / raw)
To: David Miller, ap420073; +Cc: dev, netdev, Pravin Shelar
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-08 23:22 ` Gregory Rose
@ 2019-07-11 21:07 ` Pravin Shelar
2019-07-11 23:55 ` Gregory Rose
0 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2019-07-11 21:07 UTC (permalink / raw)
To: Gregory Rose
Cc: David Miller, ap420073, ovs dev, Linux Kernel Network Developers
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
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-11 21:07 ` Pravin Shelar
@ 2019-07-11 23:55 ` Gregory Rose
0 siblings, 0 replies; 11+ messages in thread
From: Gregory Rose @ 2019-07-11 23:55 UTC (permalink / raw)
To: Pravin Shelar
Cc: David Miller, ap420073, ovs dev, Linux Kernel Network Developers
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-05 16:08 [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom Taehee Yoo
2019-07-08 23:08 ` David Miller
@ 2019-07-12 16:18 ` Gregory Rose
2019-07-12 22:18 ` David Miller
2019-07-15 18:15 ` Pravin Shelar
3 siblings, 0 replies; 11+ messages in thread
From: Gregory Rose @ 2019-07-12 16:18 UTC (permalink / raw)
To: Taehee Yoo, davem, pshelar, netdev, dev
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-05 16:08 [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom Taehee Yoo
2019-07-08 23:08 ` David Miller
2019-07-12 16:18 ` Gregory Rose
@ 2019-07-12 22:18 ` David Miller
2019-07-12 23:11 ` Gregory Rose
2019-07-15 18:15 ` Pravin Shelar
3 siblings, 1 reply; 11+ messages in thread
From: David Miller @ 2019-07-12 22:18 UTC (permalink / raw)
To: ap420073; +Cc: pshelar, netdev, dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-12 22:18 ` David Miller
@ 2019-07-12 23:11 ` Gregory Rose
0 siblings, 0 replies; 11+ messages in thread
From: Gregory Rose @ 2019-07-12 23:11 UTC (permalink / raw)
To: David Miller, ap420073; +Cc: pshelar, netdev, dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-05 16:08 [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom Taehee Yoo
` (2 preceding siblings ...)
2019-07-12 22:18 ` David Miller
@ 2019-07-15 18:15 ` Pravin Shelar
2019-07-16 12:28 ` Taehee Yoo
3 siblings, 1 reply; 11+ messages in thread
From: Pravin Shelar @ 2019-07-15 18:15 UTC (permalink / raw)
To: Taehee Yoo; +Cc: David S. Miller, Linux Kernel Network Developers, ovs dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
2019-07-15 18:15 ` Pravin Shelar
@ 2019-07-16 12:28 ` Taehee Yoo
0 siblings, 0 replies; 11+ messages in thread
From: Taehee Yoo @ 2019-07-16 12:28 UTC (permalink / raw)
To: Pravin Shelar; +Cc: David S. Miller, Linux Kernel Network Developers, ovs dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-16 12:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05 16:08 [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom Taehee Yoo
2019-07-08 23:08 ` David Miller
2019-07-08 23:18 ` [ovs-dev] " Gregory Rose
2019-07-08 23:22 ` Gregory Rose
2019-07-11 21:07 ` Pravin Shelar
2019-07-11 23:55 ` Gregory Rose
2019-07-12 16:18 ` Gregory Rose
2019-07-12 22:18 ` David Miller
2019-07-12 23:11 ` Gregory Rose
2019-07-15 18:15 ` Pravin Shelar
2019-07-16 12:28 ` Taehee Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).