Re: [PATCH] recipes: switchdev: sharedbuffer: Support pool 8

[Ondrej Lichtner]

On Tue, Oct 02, 2018 at 03:06:40PM +0000, Petr Machata wrote:
> Pool #8 was recently introduced to mlxsw to support MC-aware regime of
> the switch. Pool 8 is configured differently from the other pools, with
> infinite size, and statically-determined quotas. Tweak the sharedbuffer
> test to handle this case.
> Signed-off-by: Petr Machata
>
> recipes/switchdev/sharedbuffer.py | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/recipes/switchdev/sharedbuffer.py b/recipes/switchdev/sharedbuffer.py
> index 28a6f44..4393770 100644
> --- a/recipes/switchdev/sharedbuffer.py
> +++ b/recipes/switchdev/sharedbuffer.py
> @@ -13,6 +13,9 @@ from TestLib import TestLib
>  from time import sleep
>  import random
>
> +CELL_SIZE = 288 # Least common multiple of cell sizes of Spectrum and
>            # Spectrum-2, which are, respectively, 96 and 144.
>
> class RandomValuePicker:
>      def __init__(self, pools):
>          self._pools = {"ingress": [], "egress": []}
> @@ -27,9 +30,11 @@ class RandomValuePicker:
>          # support static threshold only for now
>          return "static"
> def _get_th(self):
>    # support dynamic threshold only for now
>    return random.randint(3,16)
>
> def _get_th(self, pool):
>    th = random.randint(3,16)
>    if pool == 8: # Pool 8 has static quotas.
>        th *= CELL_SIZE
>    return th
>
> def _get_pool(self, direction):
>        arr = self._pools[direction]
>
> @@ -40,14 +45,17 @@ class RandomValuePicker:
>              return (self._get_size(), self._get_thtype())
>          if isinstance(objid, TcBind):
>              pool = self._get_pool(objid["type"])
>        th = self._get_th()
>        th = self._get_th(pool)
>        return (pool, th)
>    if isinstance(objid, PortPool):
>        return (self._get_th(),)
>        return (self._get_th(objid["pool"]),)
>
> class RecordValuePickerException(Exception):
>      pass
>
> +class SkipTest(Exception):
> pass
>
> class RecordValuePicker:
>      def __init__(self, objlist):
>          self._recs = []
> @@ -55,6 +63,9 @@ class RecordValuePicker:
>              self._recs.append({"objid": item, "value": item.var_tuple()})
>  def get_value(self, objid):
>    if isinstance(objid, Pool) and objid["pool"] == 8:
>    # Pool 8 is reported with infinite size, which can't be reset.
>    raise SkipTest()
>    for rec in self._recs:
>        if rec["objid"].weak_eq(objid):
>            return rec["value"]
>
> @@ -124,7 +135,10 @@ def get_pools(sw, dlname, direction=None):
>  def do_check_pools(tl, sw, dlname, pools, vp):
>      for pool in pools:
>          pre_pools = get_pools(sw, dlname)
>    (size, thtype) = vp.get_value(pool)
>    try:
>        (size, thtype) = vp.get_value(pool)
>    except SkipTest:
>        continue
>    pool.dl_set(sw, dlname, size, thtype)
>    post_pools = get_pools(sw, dlname)
>    pool = post_pools.get_by(pool)
>
> -- 
> 2.4.11
> _______________________________________________
> LNST-developers mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedorahosted.org/archives/list/[e-mail]...

"8" seems like a weird magic number constant. Does it make sense to use
it in the context of an upstream repository where we want to track
generic recipes?

If it does I'm ok with pushing this.

-Ondrej

[PATCH] recipes: switchdev: sharedbuffer: Support pool 8

[Petr Machata]

Ondrej Lichtner  writes:
> On Tue, Oct 02, 2018 at 03:06:40PM +0000, Petr Machata wrote:
> > @@ -27,9 +30,11 @@ class RandomValuePicker:
> >          # support static threshold only for now
> >          return "static"
> > def _get_th(self):
> >    # support dynamic threshold only for now
> >    return random.randint(3,16)
> >
> > def _get_th(self, pool):
> >    th = random.randint(3,16)
> >    if pool == 8: # Pool 8 has static quotas.
> >        th *= CELL_SIZE
> >    return th
> >
> > def _get_pool(self, direction):
> >        arr = self._pools[direction]
> > @@ -40,14 +45,17 @@ class RandomValuePicker:
> >              return (self._get_size(), self._get_thtype())
> >          if isinstance(objid, TcBind):
> >              pool = self._get_pool(objid["type"])
> >        th = self._get_th()
> >        th = self._get_th(pool)
> >        return (pool, th)
> >    if isinstance(objid, PortPool):
> >        return (self._get_th(),)
> >        return (self._get_th(objid["pool"]),)
> >
> > class RecordValuePickerException(Exception):
> >      pass
> >
> > +class SkipTest(Exception):
> > pass
> >
> > class RecordValuePicker:
> >      def __init__(self, objlist):
> >          self._recs = []
> > @@ -55,6 +63,9 @@ class RecordValuePicker:
> >              self._recs.append({"objid": item, "value": item.var_tuple()})
> >  def get_value(self, objid):
> >    if isinstance(objid, Pool) and objid["pool"] == 8:
> >    # Pool 8 is reported with infinite size, which can't be reset.
> >    raise SkipTest()
> >    for rec in self._recs:
> >        if rec["objid"].weak_eq(objid):
> >            return rec["value"]
>
> "8" seems like a weird magic number constant. Does it make sense to use
> it in the context of an upstream repository where we want to track
> generic recipes?

Definitely not, this is specific to mlxsw. What would be a good way to
make this e.g. configurable, or somehow generic? An alias?

Thanks,
Petr

[PATCH] recipes: switchdev: sharedbuffer: Support pool 8

[Ondrej Lichtner]

On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
> Ondrej Lichtner  writes:
> > On Tue, Oct 02, 2018 at 03:06:40PM +0000, Petr Machata wrote:
> > > @@ -27,9 +30,11 @@ class RandomValuePicker:
> > >          # support static threshold only for now
> > >          return "static"
> > > def _get_th(self):
> > >    # support dynamic threshold only for now
> > >    return random.randint(3,16)
> > >
> > > def _get_th(self, pool):
> > >    th = random.randint(3,16)
> > >    if pool == 8: # Pool 8 has static quotas.
> > >        th *= CELL_SIZE
> > >    return th
> > >
> > > def _get_pool(self, direction):
> > >        arr = self._pools[direction]
> > > @@ -40,14 +45,17 @@ class RandomValuePicker:
> > >              return (self._get_size(), self._get_thtype())
> > >          if isinstance(objid, TcBind):
> > >              pool = self._get_pool(objid["type"])
> > >        th = self._get_th()
> > >        th = self._get_th(pool)
> > >        return (pool, th)
> > >    if isinstance(objid, PortPool):
> > >        return (self._get_th(),)
> > >        return (self._get_th(objid["pool"]),)
> > >
> > > class RecordValuePickerException(Exception):
> > >      pass
> > >
> > > +class SkipTest(Exception):
> > > pass
> > >
> > > class RecordValuePicker:
> > >      def __init__(self, objlist):
> > >          self._recs = []
> > > @@ -55,6 +63,9 @@ class RecordValuePicker:
> > >              self._recs.append({"objid": item, "value": item.var_tuple()})
> > >  def get_value(self, objid):
> > >    if isinstance(objid, Pool) and objid["pool"] == 8:
> > >    # Pool 8 is reported with infinite size, which can't be reset.
> > >    raise SkipTest()
> > >    for rec in self._recs:
> > >        if rec["objid"].weak_eq(objid):
> > >            return rec["value"]
> >
> > "8" seems like a weird magic number constant. Does it make sense to use
> > it in the context of an upstream repository where we want to track
> > generic recipes?
>
> Definitely not, this is specific to mlxsw. What would be a good way to
> make this e.g. configurable, or somehow generic? An alias?
>
> Thanks,
> Petr

Well, considering that the recipe is switchdev specific, if mlxsw is the
only expected implementation of it, it's probably fine to have it this
way, though it might be a good idea to have the "8" explained. Note,
this may already be the case - the commit message has some description
and the code has some additional specifics where it's used.

I'm not really familiar with switchdev so I just wanted to ask if this
is sufficient and makes sense. If it does it's ok and I can apply the
patch.

On the other hand, if you expect this switchdev recipe to run in a
non-mlxsw environment then i see two options:
* you expect that the changed pool (and therefore recipe) behaviour will
  always be present, but maybe under a different constant for different
  drivers/vendors. In this case, using an alias as a test parameter
  might make sense. This could also cover the case where the specific
  functionality isn't implemented - don't specify the alias, the
  specific recipe functionality is also skipped.
* the changed pool behaviour is mlxsw specific and will forever stay
  that way - e.g. other drivers/vendors will have different solutions to
  this and might instead want to have a different special handling of
  those (maybe even using the value "8"). In that case it would maybe
  make more sense to have multiple subclasses with the specific
  behaviour, and you could choose between them with either an alias, or
  if applicable/possible you could also detect the driver used.

I might not be explaining myself well thoug... if that's the case it
might be easier to discuss interactively over irc, feel free to ping me :).

-Ondrej

[PATCH] recipes: switchdev: sharedbuffer: Support pool 8

[Petr Machata]

Ondrej Lichtner  writes:
> On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
> > Ondrej Lichtner  writes:
> > > "8" seems like a weird magic number constant. Does it make sense to use
> > > it in the context of an upstream repository where we want to track
> > > generic recipes?
> >
> > Definitely not, this is specific to mlxsw. What would be a good way to
> > make this e.g. configurable, or somehow generic? An alias?
> >
> > Thanks,
> > Petr
>
> Well, considering that the recipe is switchdev specific, if mlxsw is the
> only expected implementation of it, it's probably fine to have it this
> way, though it might be a good idea to have the "8" explained. Note,
> this may already be the case - the commit message has some description
> and the code has some additional specifics where it's used.
>
> I'm not really familiar with switchdev so I just wanted to ask if this
> is sufficient and makes sense. If it does it's ok and I can apply the
> patch.

The recipe already makes assumptions about what the pools exported by
the driver are. As witnessed by the breakage when we added a new pool to
mlxsw :) If another driver implements devlink pools, and they want to
use this recipe, I expect they will hit another set of limitations. It's
hard to predict what those will be.
> On the other hand, if you expect this switchdev recipe to run in a
> non-mlxsw environment then i see two options:
> you expect that the changed pool (and therefore recipe) behaviour will
> always be present, but maybe under a different constant for different
> drivers/vendors. In this case, using an alias as a test parameter
> might make sense. This could also cover the case where the specific
> functionality isn't implemented - don't specify the alias, the
> specific recipe functionality is also skipped.
> the changed pool behaviour is mlxsw specific and will forever stay
> that way - e.g. other drivers/vendors will have different solutions to
> this and might instead want to have a different special handling of
> those (maybe even using the value "8"). In that case it would maybe
> make more sense to have multiple subclasses with the specific
> behaviour, and you could choose between them with either an alias, or
> if applicable/possible you could also detect the driver used.

So for the case at hand, I can determine whether a pool is static or
dynamic from "devlink sb pool", and that will get rid of one instance of
the magic 8.

Unfortunately there's no way to determine whether a static pool is
infinite or not, devlink simply doesn't have an interface for this. That
might be mlxsw-specific, I reckon. So that might be a candidate for an
alias: pass in the list of infinite pools that shouldn't be tampered
with, with the default being [].

That should make the test runnable on any driver that supports
shared-buffers, if only in reduced form where static buffers aren't
fully tested.

Does that sound acceptable?

Thanks,
Petr

[PATCH] recipes: switchdev: sharedbuffer: Support pool 8

[Ondrej Lichtner]

On Mon, Oct 08, 2018 at 02:11:55PM +0000, Petr Machata wrote:
> Ondrej Lichtner  writes:
> > On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
> > > Ondrej Lichtner  writes:
> > > > "8" seems like a weird magic number constant. Does it make sense to use
> > > > it in the context of an upstream repository where we want to track
> > > > generic recipes?
> > >
> > > Definitely not, this is specific to mlxsw. What would be a good way to
> > > make this e.g. configurable, or somehow generic? An alias?
> > >
> > > Thanks,
> > > Petr
> >
> > Well, considering that the recipe is switchdev specific, if mlxsw is the
> > only expected implementation of it, it's probably fine to have it this
> > way, though it might be a good idea to have the "8" explained. Note,
> > this may already be the case - the commit message has some description
> > and the code has some additional specifics where it's used.
> >
> > I'm not really familiar with switchdev so I just wanted to ask if this
> > is sufficient and makes sense. If it does it's ok and I can apply the
> > patch.
>
> The recipe already makes assumptions about what the pools exported by
> the driver are. As witnessed by the breakage when we added a new pool to
> mlxsw :) If another driver implements devlink pools, and they want to
> use this recipe, I expect they will hit another set of limitations. It's
> hard to predict what those will be.

yup, exactly what I meant by "I'm not really familiar with switchdev"
and I assumed that the recipes already have some assumptions in them :)
and therefore it could be ok to continue down that path until acutal
issues arise.
> > On the other hand, if you expect this switchdev recipe to run in a
> > non-mlxsw environment then i see two options:
> > you expect that the changed pool (and therefore recipe) behaviour will
> > always be present, but maybe under a different constant for different
> > drivers/vendors. In this case, using an alias as a test parameter
> > might make sense. This could also cover the case where the specific
> > functionality isn't implemented - don't specify the alias, the
> > specific recipe functionality is also skipped.
> > the changed pool behaviour is mlxsw specific and will forever stay
> > that way - e.g. other drivers/vendors will have different solutions to
> > this and might instead want to have a different special handling of
> > those (maybe even using the value "8"). In that case it would maybe
> > make more sense to have multiple subclasses with the specific
> > behaviour, and you could choose between them with either an alias, or
> > if applicable/possible you could also detect the driver used.
>
> So for the case at hand, I can determine whether a pool is static or
> dynamic from "devlink sb pool", and that will get rid of one instance of
> the magic 8.
>
> Unfortunately there's no way to determine whether a static pool is
> infinite or not, devlink simply doesn't have an interface for this. That
> might be mlxsw-specific, I reckon. So that might be a candidate for an
> alias: pass in the list of infinite pools that shouldn't be tampered
> with, with the default being [].
>
> That should make the test runnable on any driver that supports
> shared-buffers, if only in reduced form where static buffers aren't
> fully tested.
>
> Does that sound acceptable?

Sure, if you as an expert on the topic find this an improvement then
I'll accept it :). I just wanted to ask for some more info about a magic
constant.

-Ondrej