Hi Hal,
Hal Rosenstock wrote:
> On Wed, Aug 25, 2010 at 4:43 AM, Eli Dorfman (Voltaire)
> <dorfman....@gmail.com> wrote:
>> Hal Rosenstock wrote:
>>> Hi Eli,
>>>
>>> On Mon, Aug 23, 2010 at 3:37 PM, Eli Dorfman <dorfman....@gmail.com> wrote:
>>>> Hi Hal,
>>>>
>>>> On Mon, Aug 23, 2010 at 5:25 PM, Hal Rosenstock
>>>> <hal.rosenst...@gmail.com> wrote:
>>>>> On Wed, Jul 28, 2010 at 12:26 PM, Eli Dorfman (Voltaire)
>>>>> <dorfman....@gmail.com> 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 <sas...@voltaire.com>
>> 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.
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 <e...@voltaire.com>
>>>>>> ---
>>>>>> 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 majord...@vger.kernel.org
>>>>>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
