[ofa-general] [infiniband-diags] add --loop_ports option to perfquery

[Hal Rosenstock]

On Thu, Oct 9, 2008 at 1:20 PM, Al Chu <chu11 at llnl.gov> wrote:
> On Thu, 2008-10-09 at 13:13 -0400, Hal Rosenstock wrote:
>> Hi Al,
>>
>> On Thu, Oct 9, 2008 at 12:25 PM, Al Chu <chu11 at llnl.gov> wrote:
>> > Hey Hal,
>> >
>> > On Thu, 2008-10-09 at 08:37 -0400, Hal Rosenstock wrote:
>> >> Hi again Al,
>> >>
>> >> On Wed, Oct 8, 2008 at 6:45 PM, Al Chu <chu11 at llnl.gov> wrote:
>> >> > Hey Hal,
>> >> >
>> >> > On Wed, 2008-10-08 at 09:44 -0400, Hal Rosenstock wrote:
>> >> >> Hi Al,
>> >> >>
>> >> >> On Wed, Oct 8, 2008 at 6:26 AM, Al Chu <chu11 at llnl.gov> wrote:
>> >> >> > Hey Hal,
>> >> >> >
>> >> >> > On Wed, 2008-10-08 at 07:03 -0400, Hal Rosenstock wrote:
>> >> >> >> Al,
>> >> >> >>
>> >> >> >> On Tue, Oct 7, 2008 at 6:54 PM, Al Chu <chu11 at llnl.gov> wrote:
>> >> >> >> > Hey Sasha,
>> >> >> >> >
>> >> >> >> > We have a switch here that does not report the AllPortSelect flag as a
>> >> >> >> > capability.  It's pretty annoying typing each port on the switch or
>> >> >> >> > always having to script around this one oddball switch we have.  So I
>> >> >> >> > added an option --loop_ports for perfquery.  If you want to do something
>> >> >> >> > to all the ports on the CA/Switch, but AllPortSelect isn't available, it
>> >> >> >> > loops through all the available ports instead.
>> >> >> >>
>> >> >> >> Why not add simulated AllPortSelect for multiple ports rather than add
>> >> >> >> another perquery option for this ?
>> >> >> >
>> >> >> > I did try that, and it did seem to work for the switches we had.  But
>> >> >> > when I read the IB spec, it said something to the affect that if a
>> >> >> > system doesn't support AllPortSelect, setting the PortSelect field to
>> >> >> > 0xFF was undefined behavior.
>> >> >>
>> >> >> I was suggesting that the emulation support (when AllPortSelect is not
>> >> >> supported) be enhanced for multiple ports and work on both CAs and all
>> >> >> switches. The one difference is one response for AllPortSelect
>> >> >> (whether emulated or not) v. many responses for port loop.
>> >> >
>> >> > Oh.  I thought you were referring the the workaround "simulation" that
>> >> > was in the original code.  But you're referring to aggregating the
>> >> > data/output make it look like AllPortSelect was supported.  I'll put
>> >> > this on the TODO.
>> >>
>> >> So it seems that the reason for adding an additional option for this
>> >> is that the lack of this support ? Are there any other uses ?
>> >
>> > I made the --loop_ports option b/c I just didn't want to change the
>> > default behavior perfquery.  But we could easily make it an automatic if
>> > the AllPortsSelect flag isn't supported.
>> >
>> > Thinking about it, there is a bit of a subtlety in the command line
>> > options and expectations.
>> >
>> > If a user inputs a port of '255', to me this means that the user wants
>> > to do an AllPortSelect and we should error out if AllPortSelect isn't
>> > supported.
>> >
>> > If a user inputs the '-a' option, it suggests that they want perfquery
>> > to operate on every port, suggesting that we could automatically loop if
>> > they don't input --loop_ports.
>>
>> Yes, there is some redundancy now with port 255 and -a option (both do
>> the same thing) and they could be made subtly different as you
>> indicate.
>>
>> In the case of query rather than reset, we are still left with the
>> question of whether to return 1 aggregated response or 1
>> response/port. Do we need to support both ? AllPortsSelect has this as
>> aggregated.
>
> I agree that we should aggregate it.  One of the reasons I made --
> loop_ports an option was b/c I didn't want to change default behavior.
> If we're going to change the default behavior and remove the option,
> then I need to aggregate it.
>
>> I'm also not sure that the loop ports option is needed.
>>
>> IMO all that was needed was to loop on the ports when all ports is not
>> supported by the PMA and aggregate the counters and then nothing else
>> needs to change.
>
> Should we remove the previous workaround in the code?
>
>                if (allports == 1)
>                        pc[1] = ALL_PORTS;      /* fake PortSelect */
>
> Presumably we wouldn't need this anymore.  But I'm not sure if this was
> a specific need for some specific hardware or something.

That was just an easy way to say all ports for those who didn't know
255 was a magic number. That's not for any specific hardware.

The original assumption was that all PMAs supported AllPortSelect
option but that turned out not to be the case so I started adding the
emulation for a single port (which was the one case I was aware of).
Clearly, there are other cases.

-- Hal

>
> Al
>
>> -- Hal
>>
>> > Al
>> >
>> >> -- Hal
>> >>
>> >> >>
>> >> >> >> > There was already a workaround in the tool for a CA that did not support
>> >> >> >> > the AllPortSelect flag.  I get the feeling the workaround may have been
>> >> >> >> > for a specific hardware, so I kept the workaround in there.
>> >> >> >>
>> >> >> >> > Al
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > Albert Chu
>> >> >> >> > chu11 at llnl.gov
>> >> >> >> > Computer Scientist
>> >> >> >> > High Performance Systems Division
>> >> >> >> > Lawrence Livermore National Laboratory
>> >> >> >> >
>> >> >> >> > _______________________________________________
>> >> >> >> > general mailing list
>> >> >> >> > general at lists.openfabrics.org
>> >> >> >> > http://    lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>> >> >> >> >
>> >> >> >> > To unsubscribe, please visit http://    openib.org/mailman/listinfo/openib-general
>> >> >> >> >
>> >> >> >>
>> >> >> >> There are also 2 for loops which are not correct for some switches:
>> >> >> >> for (i = 1; i <= num_ports; i++)
>> >> >> >
>> >> >> > I guess I've never seen a switch that doesn't go from 1 to num_ports.
>> >> >> > Is there something else I need to handle?
>> >> >>
>> >> >> Yes, per the spec, enhanced SP0 supports PortCounters. All your
>> >> >> switches likely support AllPortSelect so it's not an issue there.
>> >> >
>> >> > Ok I see now.  Wasn't aware of it.  I'll get a patch together.
>> >> >
>> >> > Thanks,
>> >> > Al
>> >> >
>> >> >> -- Hal
>> >> >>
>> >> >> > Al
>> >> >> >
>> >> >> >> -- Hal
>> >> >> >>
>> >> >> > --
>> >> >> > Albert Chu
>> >> >> > chu11 at llnl.gov
>> >> >> > Computer Scientist
>> >> >> > High Performance Systems Division
>> >> >> > Lawrence Livermore National Laboratory
>> >> >> >
>> >> >> >
>> >> >>
>> >> > --
>> >> > Albert Chu
>> >> > chu11 at llnl.gov
>> >> > Computer Scientist
>> >> > High Performance Systems Division
>> >> > Lawrence Livermore National Laboratory
>> >> >
>> >> >
>> >>
>> > --
>> > Albert Chu
>> > chu11 at llnl.gov
>> > Computer Scientist
>> > High Performance Systems Division
>> > Lawrence Livermore National Laboratory
>> >
>> >
>>
> --
> Albert Chu
> chu11 at llnl.gov
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
>
>