| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOrHB_BXnoBsNiExF4NsDvXaLO5RAqZ7e8keLVR1Vd2z7y_sOQ@mail.gmail.com>
Date: Mon, 20 Apr 2020 14:43:33 -0700
From: Pravin Shelar <pravin.ovn@...il.com>
To: Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc: Andy Zhou <azhou@....org>, Ben Pfaff <blp@....org>,
William Tu <u9012063@...il.com>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next v2 1/5] net: openvswitch: expand the meters
supported number
On Sun, Apr 19, 2020 at 5:23 PM Tonghao Zhang <xiangxia.m.yue@...il.com> wrote:
>
> On Mon, Apr 20, 2020 at 1:29 AM Pravin Shelar <pravin.ovn@...il.com> wrote:
> >
> > On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m.yue@...il.com> wrote:
> > >
> > > From: Tonghao Zhang <xiangxia.m.yue@...il.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@....org>
> > > Cc: Andy Zhou <azhou@....org>
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.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.
Powered by blists - more mailing lists