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".
> 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.
>>
>>> 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 ?
>
>>
>> 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.
>>
>> 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
