Hi Eli, On Thu, Aug 26, 2010 at 2:24 AM, Eli Dorfman (Voltaire) <[email protected]> wrote: > Hi Hal, > > Hal Rosenstock wrote: >> On Wed, Aug 25, 2010 at 4:43 AM, Eli Dorfman (Voltaire) >> <[email protected]> wrote: >>> Hal Rosenstock wrote: >>>> Hi Eli, >>>> >>>> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <[email protected]> wrote: >>>>> Hi Hal, >>>>> >>>>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock >>>>> <[email protected]> wrote: >>>>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire) >>>>>> <[email protected]> wrote: >>>>>>> Subject: [PATCH] Fix sl2vl configuration >>>>>>> >>>>>>> For non-optimized sl2vl configuration in and out ports were reversed. >>>>>> Are you sure these are reversed ? Any idea which commit introduced >>>>>> this reversal of in and out ports ? >>>>> I'm sure they are reversed. >>>> I looked at it some more and the ports look reversed to me too. >>>> >>>>> This patch was also tested by Jim Schutt. >>>> That only means it works in his environment rather than "correctness". >>> It was tested in mine enviornment as well. >>> Usually I test the patch in my environment to verify its correctness (in >>> addition to code review). >>> I assume you do the same. >>> >>> Do you expect me to test the patch in your environment as well? >> >> Huh ? >> >>>>> I didn't check which commit introduced this - why is it important? >>>> I'd like to understand which patch introduced the reversal. I don't >>>> see it but might have missed it. I think it's important to know which >>>> versions are broken. >>> I think that the following commit added the bug: >>> >>> commit 051a1dd514e63f3a971afad38e377932efca5e18 >>> Author: Sasha Khapyorsky <[email protected]> >>> Date: Mon Jan 4 21:06:19 2010 +0200 >>> >>> opensm/osm_qos.c: split switch external and end ports setup >>> >>> This splits QoS related port parameters setup over switch external ports >>> and end ports. Such separation leaves us with simpler code and saves >>> some repeated flows in case of switch external ports (actually required >>> per switch and not per port). >>> >>> Another advantages: Optimized Sl2VL Mapping procedure can be implemented >>> easier using this model. A low level port QoS related parameters setup >>> infrastructure is ready for supporting per port QoS related configuration >>> (which hopefully will be implemented some days). >>> >> >> I thought it might be that one but wasn't sure. Thanks. >> >>>>>>> For optimal sl2vl added override of default ALL settting with port's >>>>>>> sl2vl when operational VL was other than the default port. >>>>>> What is the motivation to override when the operational VLs is >>>>>> different ? Why is that better than what is done currently ? >>>>> The idea was to apply the default policy - set sl2vl modulo operational >>>>> VL. >>>>> When applying ALL settings using port 1 we still want to override this >>>>> setting for ports with different operational VL. >>>> What makes the default policy modulo operational VLs ? >>> This is how its implemented in sl2vl_update_table() >>> vl_mask = (1 << (ib_port_info_get_op_vls(&p->port_info) - 1)) - 1; >>> >>> for (i = 0; i < IB_MAX_NUM_VLS / 2; i++) { >>> vl1 = sl2vl_table->raw_vl_by_sl[i] >> 4; >>> vl2 = sl2vl_table->raw_vl_by_sl[i] & 0xf; >>> if (vl1 != 15) >>> vl1 &= vl_mask; >>> if (vl2 != 15) >>> vl2 &= vl_mask; >>> >>> Default startup switches configuration uses the same policy. >> >> I view this as further modification on the configuration based on the >> operational VLs. Besides, any VL specified above the op VLs is a drop >> (same as indicating VL15). > > This code is already in the trunk and my patch does not change that. > The only diff in the patch is the fix of reversed in/out ports and > corresponding fix for > optimized sl2vl configuration. > >> >> I think what you are doing is changing the default behavior and hence >> perhaps an additional policy switch is needed if this change really is >> needed and I'm unsure of that. > > I don't change the default behavior and this change is needed. > Without it for example, if you connect a switch with opVL=4 (VL0-VL7) to a > node that > supports only VL0 and try to send traffic on sl>SL0 it will be dropped.
Yes, you are right. I stand corrected. -- Hal > Eli > >> >>>>>> Is this really a policy issue ? >>>>>> >>>>>> IMO there are two separate issues in this patch and they should be in >>>>>> separate patches (for better git bisection). >>>>> Maybe, but I still think this patch fixes wrong setting for sl2vl >>>>> using optimized and non optimized methods. >>>>> I'm not sure this should be divided to 2 separate patches. >>>> It's one thought per patch and to me this is two different thoughts. >>> If this is the only issue, I can split this patch to 2 separate patches. >> >> It's also the issue noted above. >> >> -- Hal >> >>> Eli >>>>>> Also, a couple of (possibly related) questions below. >>>>> It seems that you are referring to patch v1 which was modified >>>>> according to Jim's comments. >>>>> Please check the v2 patch . >>>> I see my questions are moot in terms of the v2 patch. >>>> >>>> -- Hal >>>> >>>>> Thanks, >>>>> Eli >>>>> >>>>>>> Signed-off-by: Eli Dorfman <[email protected]> >>>>>>> --- >>>>>>> opensm/opensm/osm_qos.c | 25 ++++++++++++++++++++----- >>>>>>> 1 files changed, 20 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c >>>>>>> index a571370..de0ae23 100644 >>>>>>> --- a/opensm/opensm/osm_qos.c >>>>>>> +++ b/opensm/opensm/osm_qos.c >>>>>>> @@ -182,7 +182,7 @@ static ib_api_status_t sl2vl_update_table(osm_sm_t >>>>>>> * sm, osm_physp_t * p, >>>>>>> tbl.raw_vl_by_sl[i] = (vl1 << 4) | vl2; >>>>>>> } >>>>>>> >>>>>>> - if (!force_update && (p_tbl = osm_physp_get_slvl_tbl(p, >>>>>>> in_port)) && >>>>>>> + if (!force_update && in_port && (p_tbl = >>>>>>> osm_physp_get_slvl_tbl(p, in_port)) && >>>>>>> !memcmp(p_tbl, &tbl, sizeof(tbl))) >>>>>>> return IB_SUCCESS; >>>>>> Why exclude port 0 here ? Is it related to the change noted below ? >>>>>> >>>>>>> @@ -209,6 +209,7 @@ static int qos_extports_setup(osm_sm_t * sm, >>>>>>> osm_node_t *node, >>>>>>> unsigned num_ports = osm_node_get_num_physp(node); >>>>>>> int ret = 0; >>>>>>> unsigned i, j; >>>>>>> + uint8_t op_vl1; >>>>>>> >>>>>>> for (i = 1; i < num_ports; i++) { >>>>>>> p = osm_node_get_physp_ptr(node, i); >>>>>>> @@ -225,17 +226,31 @@ static int qos_extports_setup(osm_sm_t * sm, >>>>>>> osm_node_t *node, >>>>>>> if (ib_switch_info_get_opt_sl2vlmapping(&node->sw->switch_info) >>>>>>> && >>>>>>> sm->p_subn->opt.use_optimized_slvl) { >>>>>>> p = osm_node_get_physp_ptr(node, 1); >>>>>>> + op_vl1 = ib_port_info_get_op_vls(&p->port_info); >>>>>>> force_update = p->need_update || sm->p_subn->need_update; >>>>>>> - return sl2vl_update_table(sm, p, 1, 0x30000, >>>>>>> force_update, >>>>>>> - &qcfg->sl2vl); >>>>>>> + if (sl2vl_update_table(sm, p, 0, 0x30000, force_update, >>>>>> Why is the third parameter (in_port) changed from 1 to 0 here ? Maybe >>>>>> that's related to the change above for the skipping of port 0 in >>>>>> sl2vl_update_table. >>>>>> >>>>>> -- Hal >>>>>> >>>>>>> + &qcfg->sl2vl)) >>>>>>> + ret = -1; >>>>>>> + /* overwrite default ALL configuration if port's >>>>>>> + op_vl is different */ >>>>>>> + for (i = 2; i < num_ports; i++) { >>>>>>> + p = osm_node_get_physp_ptr(node, i); >>>>>>> + if (ib_port_info_get_op_vls(&p->port_info) != >>>>>>> op_vl1 && >>>>>>> + sl2vl_update_table(sm, p, 0, 0x20000 | i, >>>>>>> force_update, >>>>>>> + &qcfg->sl2vl)) >>>>>>> + ret = -1; >>>>>>> + } >>>>>>> + return ret; >>>>>>> } >>>>>>> >>>>>>> - for (i = 0; i < num_ports; i++) { >>>>>>> + /* non optimized sl2vl configuration */ >>>>>>> + i = ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? >>>>>>> 0 : 1; >>>>>>> + for (; i < num_ports; i++) { >>>>>>> p = osm_node_get_physp_ptr(node, i); >>>>>>> force_update = p->need_update || sm->p_subn->need_update; >>>>>>> j = >>>>>>> ib_switch_info_is_enhanced_port0(&node->sw->switch_info) ? 0 : 1; >>>>>>> for (; j < num_ports; j++) >>>>>>> - if (sl2vl_update_table(sm, p, i, i << 8 | j, >>>>>>> + if (sl2vl_update_table(sm, p, j, j << 8 | i, >>>>>>> force_update, >>>>>>> &qcfg->sl2vl)) >>>>>>> ret = -1; >>>>>>> } >>>>>>> -- >>>>>>> 1.5.5 >>>>>>> >>>>>>> -- >>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >>>>>>> the body of a message to [email protected] >>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>>> >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
