Message-ID: <20190330073527.GB14297@nanopsycho>
Date:   Sat, 30 Mar 2019 08:35:27 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, mlxsw@...lanox.com,
        Ido Schimmel <idosch@...lanox.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, vivien.didelot@...il.com,
        Michael Chan <michael.chan@...adcom.com>,
        Or Gerlitz <ogerlitz@...lanox.com>
Subject: Re: [patch net-next 00/12] net: expose switch ID via devlink

Fri, Mar 29, 2019 at 11:34:14PM CET, jakub.kicinski@...ronome.com wrote:
>On Fri, Mar 29, 2019 at 2:21 PM Jiri Pirko <jiri@...nulli.us> wrote:
>> Fri, Mar 29, 2019 at 07:59:26PM CET, jakub.kicinski@...ronome.com wrote:
>> >On Fri, 29 Mar 2019 07:49:05 +0100, Jiri Pirko wrote:
>> >> Thu, Mar 28, 2019 at 10:40:02PM CET, jakub.kicinski@...ronome.com wrote:
>> >> >On Thu, 28 Mar 2019 22:12:42 +0100, Jiri Pirko wrote:
>> >> >> From: Jiri Pirko <jiri@...lanox.com>
>> >> >>
>> >> >> To provide visibility of the ports, this patchset exposes switch ID
>> >> >> for devlink ports, which are part of a switch. The rest of the ports
>> >> >> if any (in case of sr-iov for example) do not set switch ID.
>> >> >
>> >> >I don't feel good about this patch set.  There is no visibility
>> >> >provided here.  Should the port flavour should be a sufficient
>> >>
>> >> 1) this patch is mainly about avoiding need to define the ndo and moving
>> >>    the switch id definition to devlink port attr.
>> >
>> >Sure, that you could achieve by putting the data in the netdevice
>> >structure as well..
>> >
>> >What is the guiding principle here?  I'm trying to argue for leaving
>> >forwarding-related info in netdev code, and only have HW control in
>> >devlink.  I just don't see switch id being useful at devlink level in
>> >any way.
>>
>> Well we have switchib driver which does not have any netdevice and still
>> the ports are part of a switch. In other words, this is not
>> ethernet-specific attribute, therefore devlink is the right fit.
>
>Ack, but switch id is a switchdev thing, controlling which ports are
>considered to be part of the same device _for_ _switchdev_.

"Switchdev" is a bridge offload. Nothing else.


>
>Is switchib doing forwarding?  Not long ago Parav was convincing us
>that switchdev mode for IB is pretty much meaningless.  Even though
>there are apparently representors for IB (judging by the recent RDMA
>patchset on netdev)...

Switch id is simply identifier for ports in general that belong to some
switch. No matter if ethernet or IB.


>
>> >> 2) along with that, switch id is added as attribute. It tells the user
>> >>    that some devlink port is part of a switch with certain id. If port
>> >>    is not part of a switch (like upcoming hostport, cpu, dsa, etc),
>> >>    switch id is not set on that port
>> >
>> >If the flavour already gives that information, why crowd the attributes
>> >for ports with switch id?
>>
>> Hmm, we'll have multiple non-switch port flavours and once your vf/pf
>> patchset  hits the tree we'll have multipkle switch port flavours.
>> So makes sense to have switch id.
>
>It's probably a matter of preference then.  To me its very clear to say that
>you can switch to physical/vf/pf ports and not cpu/dsa/host. I don't like
>the duplication of dumpnig the same attribute both in rtnetlink and devlink.

I want to remind you, that switch_id in rtnetlink was introduced before
devlink existed. Back than, if was the only place to put it. However, as
I explained, it is not ethernet speficic thing and rather belongs to
devlink. This patchset is doing that.


>
>> Also, you can have multiple switches within one asic.
>
>That line is blurry.

It is. So far, it is mlx CX hw that have basically 2 switches, one per
PF.


>
>> >> >indication of whether netdev associated with that port can be
>> >> >switched to or not?  CPU, DSA, and Host flavours can't be switched
>> >> >to.  And the switchid can be an attribute of the devlink instance,
>> >> >if we want to expose it via devlink.
>> >>
>> >> One devlink instance can have multiple switch ids in use as it may
>> >> contain multiple switches. Take mlx5 as an instance. Currently every PF
>> >> creates a separate devlink instance, however there are some features
>> >> shared. In this example, with proposed idea of aliasing, there would be
>> >> one devlink instance aliased between these 2 pf inctances, with 2
>> >> eswitches and 2 sets of switch ports each belonging to an eswitch -
>> >> distinguished by switch id.
>> >
>> >Out of curiosity, what are the shared features?  It seems mlx5 drives
>> >a lot of our API design, it'd be good if the community had a better
>> >understanding of it.
>>
>> I have to gather that info. Not so many things are shared. There is one
>> extra switch to mix 2 pfs together. I know about some IB features that
>> also mix 2 pfs.
>
>I see, so a dual port IB card would somehow have only one whatever the
>equivalent of a netdev in IB world is?

Yes. We have a patchset internally to manage that.


>
>> >The situation with pipelined devices is somewhat murky.  Didn't Or add
>> >some from of PCIe-side looped queue to forward between PFs?
>>
>> I have no clue. Ccing Or. Or?
>>
>> >Presumably DSA would lean the opposite way with multiple ASICs
>> >reporting the same ID?
>>
>> Yes, sounds right.
>
>Right, so to me switch ID really is a forwarding attribute.

Powered by blists - more mailing lists