| Message ID | 20190705160809.5202-1-ap420073@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [ovs-dev,net-next] net: openvswitch: do not update max_headroom if new headroom is equal to old headroom | expand |
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.
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
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 >
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 > > >
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
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>
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.
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
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.
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.
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);
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(-)