[ovs-dev] [PATCH net-next v2 1/5] net: openvswitch: expand the meters supported number

[Pravin Shelar]

On Sun, Apr 19, 2020 at 5:23 PM Tonghao Zhang <xiangxia.m.yue at gmail.com> wrote:
>
> On Mon, Apr 20, 2020 at 1:29 AM Pravin Shelar <pravin.ovn at gmail.com> wrote:
> >
> > On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m.yue at gmail.com> wrote:
> > >
> > > From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > >
> > > In kernel datapath of Open vSwitch, there are only 1024
> > > buckets of meter in one dp. If installing more than 1024
> > > (e.g. 8192) meters, it may lead to the performance drop.
> > > But in some case, for example, Open vSwitch used as edge
> > > gateway, there should be 200,000+ at least, meters used for
> > > IP address bandwidth limitation.
> > >
> > > [Open vSwitch userspace datapath has this issue too.]
> > >
> > > For more scalable meter, this patch expands the buckets
> > > when necessary, so we can install more meters in the datapath.
> > > Introducing the struct *dp_meter_instance*, it's easy to
> > > expand meter though changing the *ti* point in the struct
> > > *dp_meter_table*.
> > >
> > > Cc: Pravin B Shelar <pshelar at ovn.org>
> > > Cc: Andy Zhou <azhou at ovn.org>
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> > > ---
> > >  net/openvswitch/datapath.h |   2 +-
> > >  net/openvswitch/meter.c    | 200 +++++++++++++++++++++++++++++--------
> > >  net/openvswitch/meter.h    |  15 ++-
> > >  3 files changed, 169 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> > > index e239a46c2f94..785105578448 100644
> > > --- a/net/openvswitch/datapath.h
> > > +++ b/net/openvswitch/datapath.h
> > > @@ -82,7 +82,7 @@ struct datapath {
> > >         u32 max_headroom;
> > >
> > >         /* Switch meters. */
> > > -       struct hlist_head *meters;
> > > +       struct dp_meter_table *meters;
> > lets define it as part of this struct to avoid indirection.
> >
> > >  };
> > >
> > >  /**
> > > diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
> > > index 5010d1ddd4bd..494a0014ecd8 100644
> > > --- a/net/openvswitch/meter.c
> > > +++ b/net/openvswitch/meter.c
> > > @@ -19,8 +19,6 @@
> > >  #include "datapath.h"
> > >  #include "meter.h"
> > >
> > > -#define METER_HASH_BUCKETS 1024
> > > -
> > >  static const struct nla_policy meter_policy[OVS_METER_ATTR_MAX + 1] = {
> > >         [OVS_METER_ATTR_ID] = { .type = NLA_U32, },
> > >         [OVS_METER_ATTR_KBPS] = { .type = NLA_FLAG },
> > > @@ -39,6 +37,11 @@ static const struct nla_policy band_policy[OVS_BAND_ATTR_MAX + 1] = {
> > >         [OVS_BAND_ATTR_STATS] = { .len = sizeof(struct ovs_flow_stats) },
> > >  };
> > >
> > > +static u32 meter_hash(struct dp_meter_instance *ti, u32 id)
> > > +{
> > > +       return id % ti->n_meters;
> > > +}
> > > +
> > >  static void ovs_meter_free(struct dp_meter *meter)
> > >  {
> > >         if (!meter)
> > > @@ -47,40 +50,141 @@ static void ovs_meter_free(struct dp_meter *meter)
> > >         kfree_rcu(meter, rcu);
> > >  }
> > >
> > > -static struct hlist_head *meter_hash_bucket(const struct datapath *dp,
> > > -                                           u32 meter_id)
> > > -{
> > > -       return &dp->meters[meter_id & (METER_HASH_BUCKETS - 1)];
> > > -}
> > > -
> > >  /* Call with ovs_mutex or RCU read lock. */
> > > -static struct dp_meter *lookup_meter(const struct datapath *dp,
> > > +static struct dp_meter *lookup_meter(const struct dp_meter_table *tbl,
> > >                                      u32 meter_id)
> > >  {
> > > +       struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti);
> > > +       u32 hash = meter_hash(ti, meter_id);
> > >         struct dp_meter *meter;
> > > -       struct hlist_head *head;
> > >
> > > -       head = meter_hash_bucket(dp, meter_id);
> > > -       hlist_for_each_entry_rcu(meter, head, dp_hash_node,
> > > -                               lockdep_ovsl_is_held()) {
> > > -               if (meter->id == meter_id)
> > > -                       return meter;
> > > -       }
> > > +       meter = rcu_dereference_ovsl(ti->dp_meters[hash]);
> > > +       if (meter && likely(meter->id == meter_id))
> > > +               return meter;
> > > +
> > >         return NULL;
> > >  }
> > >
> > > -static void attach_meter(struct datapath *dp, struct dp_meter *meter)
> > > +static struct dp_meter_instance *dp_meter_instance_alloc(const u32 size)
> > > +{
> > > +       struct dp_meter_instance *ti;
> > > +
> > > +       ti = kvzalloc(sizeof(*ti) +
> > > +                     sizeof(struct dp_meter *) * size,
> > > +                     GFP_KERNEL);
> > > +       if (!ti)
> > > +               return NULL;
> > Given this is a kernel space array we need to have hard limit inplace.
> In patch 2, I limited the meter number, should we add hard limit here ?
I guess its not needed here.
...

> > >  static struct sk_buff *
> > > @@ -303,9 +407,13 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
> > >         meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> > >
> > >         /* Cannot fail after this. */
> > > -       old_meter = lookup_meter(dp, meter_id);
> > > -       detach_meter(old_meter);
> > > -       attach_meter(dp, meter);
> > > +       old_meter = lookup_meter(dp->meters, meter_id);
> > in new scheme this can fail due to hash collision, lets check for NULL.
> If old_meter is NULL, detach_meter will do nothing.

Lets return error.