Message ID | 1587032223-49460-3-git-send-email-xiangxia.m.yue@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | expand meter tables and fix bug | expand |
On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Don't allow user to create meter unlimitedly, > which may cause to consume a large amount of kernel memory. > The 200,000 meters may be fine in general case. > > Cc: Pravin B Shelar <pshelar@ovn.org> > Cc: Andy Zhou <azhou@ovn.org> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > net/openvswitch/meter.c | 21 +++++++++++++++------ > net/openvswitch/meter.h | 1 + > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c > index 494a0014ecd8..1b6776f9c109 100644 > --- a/net/openvswitch/meter.c > +++ b/net/openvswitch/meter.c > @@ -137,6 +137,7 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > { > struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti); > u32 hash = meter_hash(ti, meter->id); > + int err; > > /* > * In generally, slot selected should be empty, because > @@ -148,16 +149,24 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > dp_meter_instance_insert(ti, meter); > > /* That function is thread-safe. */ > - if (++tbl->count >= ti->n_meters) > - if (dp_meter_instance_realloc(tbl, ti->n_meters * 2)) > - goto expand_err; > + tbl->count++; > + if (tbl->count > DP_METER_NUM_MAX) { > + err = -EFBIG; > + goto attach_err; > + } > + > + if (tbl->count >= ti->n_meters && > + dp_meter_instance_realloc(tbl, ti->n_meters * 2)) { > + err = -ENOMEM; > + goto attach_err; > + } > > return 0; > > -expand_err: > +attach_err: > dp_meter_instance_remove(ti, meter); > tbl->count--; > - return -ENOMEM; > + return err; > } > > static void detach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > @@ -264,7 +273,7 @@ static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info) > if (IS_ERR(reply)) > return PTR_ERR(reply); > > - if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) || > + if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, DP_METER_NUM_MAX) || > nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS)) > goto nla_put_failure; > > diff --git a/net/openvswitch/meter.h b/net/openvswitch/meter.h > index d91940383bbe..cdfc6b9dbd42 100644 > --- a/net/openvswitch/meter.h > +++ b/net/openvswitch/meter.h > @@ -19,6 +19,7 @@ struct datapath; > > #define DP_MAX_BANDS 1 > #define DP_METER_ARRAY_SIZE_MIN (1ULL << 10) > +#define DP_METER_NUM_MAX (200000ULL) > Lets make it configurable and default could 200k to allow customization on different memory configurations. > struct dp_meter_band { > u32 type; > -- > 2.23.0 >
On Mon, Apr 20, 2020 at 1:31 AM Pravin Shelar <pravin.ovn@gmail.com> wrote: > > On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > Don't allow user to create meter unlimitedly, > > which may cause to consume a large amount of kernel memory. > > The 200,000 meters may be fine in general case. > > > > Cc: Pravin B Shelar <pshelar@ovn.org> > > Cc: Andy Zhou <azhou@ovn.org> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > net/openvswitch/meter.c | 21 +++++++++++++++------ > > net/openvswitch/meter.h | 1 + > > 2 files changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c > > index 494a0014ecd8..1b6776f9c109 100644 > > --- a/net/openvswitch/meter.c > > +++ b/net/openvswitch/meter.c > > @@ -137,6 +137,7 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > { > > struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti); > > u32 hash = meter_hash(ti, meter->id); > > + int err; > > > > /* > > * In generally, slot selected should be empty, because > > @@ -148,16 +149,24 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > dp_meter_instance_insert(ti, meter); > > > > /* That function is thread-safe. */ > > - if (++tbl->count >= ti->n_meters) > > - if (dp_meter_instance_realloc(tbl, ti->n_meters * 2)) > > - goto expand_err; > > + tbl->count++; > > + if (tbl->count > DP_METER_NUM_MAX) { > > + err = -EFBIG; > > + goto attach_err; > > + } > > + > > + if (tbl->count >= ti->n_meters && > > + dp_meter_instance_realloc(tbl, ti->n_meters * 2)) { > > + err = -ENOMEM; > > + goto attach_err; > > + } > > > > return 0; > > > > -expand_err: > > +attach_err: > > dp_meter_instance_remove(ti, meter); > > tbl->count--; > > - return -ENOMEM; > > + return err; > > } > > > > static void detach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > @@ -264,7 +273,7 @@ static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info) > > if (IS_ERR(reply)) > > return PTR_ERR(reply); > > > > - if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) || > > + if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, DP_METER_NUM_MAX) || > > nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS)) > > goto nla_put_failure; > > > > diff --git a/net/openvswitch/meter.h b/net/openvswitch/meter.h > > index d91940383bbe..cdfc6b9dbd42 100644 > > --- a/net/openvswitch/meter.h > > +++ b/net/openvswitch/meter.h > > @@ -19,6 +19,7 @@ struct datapath; > > > > #define DP_MAX_BANDS 1 > > #define DP_METER_ARRAY_SIZE_MIN (1ULL << 10) > > +#define DP_METER_NUM_MAX (200000ULL) > > > Lets make it configurable and default could 200k to allow > customization on different memory configurations. Great, set different limit depend on current system memory size like tcp ? > > > struct dp_meter_band { > > u32 type; > > -- > > 2.23.0 > >
On Sun, Apr 19, 2020 at 5:28 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Mon, Apr 20, 2020 at 1:31 AM Pravin Shelar <pravin.ovn@gmail.com> wrote: > > > > On Sat, Apr 18, 2020 at 10:25 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > Don't allow user to create meter unlimitedly, > > > which may cause to consume a large amount of kernel memory. > > > The 200,000 meters may be fine in general case. > > > > > > Cc: Pravin B Shelar <pshelar@ovn.org> > > > Cc: Andy Zhou <azhou@ovn.org> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > --- > > > net/openvswitch/meter.c | 21 +++++++++++++++------ > > > net/openvswitch/meter.h | 1 + > > > 2 files changed, 16 insertions(+), 6 deletions(-) > > > > > > diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c > > > index 494a0014ecd8..1b6776f9c109 100644 > > > --- a/net/openvswitch/meter.c > > > +++ b/net/openvswitch/meter.c > > > @@ -137,6 +137,7 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > > { > > > struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti); > > > u32 hash = meter_hash(ti, meter->id); > > > + int err; > > > > > > /* > > > * In generally, slot selected should be empty, because > > > @@ -148,16 +149,24 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > > dp_meter_instance_insert(ti, meter); > > > > > > /* That function is thread-safe. */ > > > - if (++tbl->count >= ti->n_meters) > > > - if (dp_meter_instance_realloc(tbl, ti->n_meters * 2)) > > > - goto expand_err; > > > + tbl->count++; > > > + if (tbl->count > DP_METER_NUM_MAX) { > > > + err = -EFBIG; > > > + goto attach_err; > > > + } > > > + > > > + if (tbl->count >= ti->n_meters && > > > + dp_meter_instance_realloc(tbl, ti->n_meters * 2)) { > > > + err = -ENOMEM; > > > + goto attach_err; > > > + } > > > > > > return 0; > > > > > > -expand_err: > > > +attach_err: > > > dp_meter_instance_remove(ti, meter); > > > tbl->count--; > > > - return -ENOMEM; > > > + return err; > > > } > > > > > > static void detach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) > > > @@ -264,7 +273,7 @@ static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info) > > > if (IS_ERR(reply)) > > > return PTR_ERR(reply); > > > > > > - if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) || > > > + if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, DP_METER_NUM_MAX) || > > > nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS)) > > > goto nla_put_failure; > > > > > > diff --git a/net/openvswitch/meter.h b/net/openvswitch/meter.h > > > index d91940383bbe..cdfc6b9dbd42 100644 > > > --- a/net/openvswitch/meter.h > > > +++ b/net/openvswitch/meter.h > > > @@ -19,6 +19,7 @@ struct datapath; > > > > > > #define DP_MAX_BANDS 1 > > > #define DP_METER_ARRAY_SIZE_MIN (1ULL << 10) > > > +#define DP_METER_NUM_MAX (200000ULL) > > > > > Lets make it configurable and default could 200k to allow > > customization on different memory configurations. > Great, set different limit depend on current system memory size like tcp ? Yes. that could be useful.
diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c index 494a0014ecd8..1b6776f9c109 100644 --- a/net/openvswitch/meter.c +++ b/net/openvswitch/meter.c @@ -137,6 +137,7 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) { struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti); u32 hash = meter_hash(ti, meter->id); + int err; /* * In generally, slot selected should be empty, because @@ -148,16 +149,24 @@ static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) dp_meter_instance_insert(ti, meter); /* That function is thread-safe. */ - if (++tbl->count >= ti->n_meters) - if (dp_meter_instance_realloc(tbl, ti->n_meters * 2)) - goto expand_err; + tbl->count++; + if (tbl->count > DP_METER_NUM_MAX) { + err = -EFBIG; + goto attach_err; + } + + if (tbl->count >= ti->n_meters && + dp_meter_instance_realloc(tbl, ti->n_meters * 2)) { + err = -ENOMEM; + goto attach_err; + } return 0; -expand_err: +attach_err: dp_meter_instance_remove(ti, meter); tbl->count--; - return -ENOMEM; + return err; } static void detach_meter(struct dp_meter_table *tbl, struct dp_meter *meter) @@ -264,7 +273,7 @@ static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info) if (IS_ERR(reply)) return PTR_ERR(reply); - if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) || + if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, DP_METER_NUM_MAX) || nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS)) goto nla_put_failure; diff --git a/net/openvswitch/meter.h b/net/openvswitch/meter.h index d91940383bbe..cdfc6b9dbd42 100644 --- a/net/openvswitch/meter.h +++ b/net/openvswitch/meter.h @@ -19,6 +19,7 @@ struct datapath; #define DP_MAX_BANDS 1 #define DP_METER_ARRAY_SIZE_MIN (1ULL << 10) +#define DP_METER_NUM_MAX (200000ULL) struct dp_meter_band { u32 type;