3:46 a.m.
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 <petrm(a)mellanox.com>
---
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 -- lnst-developers(a)lists.fedorahosted.org
To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
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/lnst-developers@lists.fedora...
"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
5:23 a.m.
New subject: [PATCH] recipes: switchdev: sharedbuffer: Support pool 8
Ondrej Lichtner <olichtne(a)redhat.com> 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
7:10 a.m.
New subject: [PATCH] recipes: switchdev: sharedbuffer: Support pool 8
On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
Ondrej Lichtner <olichtne(a)redhat.com> 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
9:11 a.m.
New subject: [PATCH] recipes: switchdev: sharedbuffer: Support pool 8
Ondrej Lichtner <olichtne(a)redhat.com> writes:
On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
> Ondrej Lichtner <olichtne(a)redhat.com> 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
2:06 a.m.
New subject: [PATCH] recipes: switchdev: sharedbuffer: Support pool 8
On Mon, Oct 08, 2018 at 02:11:55PM +0000, Petr Machata wrote:
Ondrej Lichtner <olichtne(a)redhat.com> writes:
> On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
>> Ondrej Lichtner <olichtne(a)redhat.com> 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
2026
days inactive
2030
days old
lnst-developers@lists.fedorahosted.org
4 comments
2 participants
participants (2)
-
Ondrej Lichtner -
Petr Machata