On Sun, Apr 19, 2020 at 5:23 PM Tonghao Zhang <xiangxia.m....@gmail.com> wrote: > > On Mon, Apr 20, 2020 at 1:29 AM Pravin Shelar <pravin....@gmail.com> wrote: > > > > On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m....@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m....@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 <pshe...@ovn.org> > > > Cc: Andy Zhou <az...@ovn.org> > > > Signed-off-by: Tonghao Zhang <xiangxia.m....@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev