Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
On Tue, 16 Jul 2019 at 03:15, Pravin Shelar wrote: > Hi Pravin, > On Fri, Jul 5, 2019 at 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 > > 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
On Fri, Jul 5, 2019 at 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 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
On 7/12/2019 3:18 PM, David Miller wrote: From: Taehee Yoo 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 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
From: Taehee Yoo 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 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
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
---
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, >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(_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
Reviewed-by: Greg Rose
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
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 wrote: On 7/8/2019 4:18 PM, Gregory Rose wrote: On 7/8/2019 4:08 PM, David Miller wrote: From: Taehee Yoo 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 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
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 wrote: > > > > On 7/8/2019 4:18 PM, Gregory Rose wrote: > > On 7/8/2019 4:08 PM, David Miller wrote: > >> From: Taehee Yoo > >> 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 > >> 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 > >> d...@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
On 7/8/2019 4:18 PM, Gregory Rose wrote: On 7/8/2019 4:08 PM, David Miller wrote: From: Taehee Yoo 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 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
On 7/8/2019 4:08 PM, David Miller wrote: From: Taehee Yoo 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 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
From: Taehee Yoo 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 I'm not so sure about the logic here and I'd therefore like an OVS expert to review this. Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom
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
---
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, >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(_vport_genl_family, reply, info);
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
