Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jiri Pirko <jiri@resnulli.us>
To: Parav Pandit <parav@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Parav Pandit <parav@mellanox.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"roid@mellanox.com" <roid@mellanox.com>,
	"saeedm@mellanox.com" <saeedm@mellanox.com>,
	Jiri Pirko <jiri@nvidia.com>
Subject: Re: [PATCH net-next 2/3] devlink: Consider other controller while building phys_port_name
Date: Tue, 1 Sep 2020 11:17:42 +0200	[thread overview]
Message-ID: <20200901091742.GF3794@nanopsycho.orion> (raw)
In-Reply-To: <BY5PR12MB43229CA19D3D8215BC9BEFECDC2E0@BY5PR12MB4322.namprd12.prod.outlook.com>

Tue, Sep 01, 2020 at 10:53:23AM CEST, parav@nvidia.com wrote:
>
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Tuesday, September 1, 2020 1:49 PM
>> 
>> >> > How? How do we tell that pfnum A means external system.
>> >> > Want to avoid such 'implicit' notion.
>> >>
>> >> How do you tell that controller A means external system?
>> 
>> Perhaps the attr name could be explicitly containing "external" word?
>> Like:
>> "ext_controller" or "extnum" (similar to "pfnum" and "vfnum") something
>> like that.
>
>How about ecnum "external controller number"?
>Tiny change in the phys_port_name below example.
>
>> 
>> 
>> >Which is why I started with annotating only external controllers, mainly to
>> avoid renaming and breaking current scheme for non_smartnic cases which
>> possibly is the most user base.
>> >
>> >But probably external pcipf/vf/sf port flavours are more intuitive combined
>> with controller number.
>> >More below.
>> >
>> >>
>> >> > > > > I can see how having multiple controllers may make things
>> >> > > > > clearer, but adding another layer of IDs while the one under
>> >> > > > > it is unused
>> >> > > > > (pfnum=0) feels very unnecessary.
>> >> > > > pfnum=0 is used today. not sure I understand your comment about
>> >> > > > being unused. Can you please explain?
>> >> > >
>> >> > > You examples only ever have pfnum 0:
>> >> > >
>> >> > Because both controllers have pfnum 0.
>> >> >
>> >> > > From patch 2:
>> >> > >
>> >> > > $ devlink port show pci/0000:00:08.0/2
>> >> > > pci/0000:00:08.0/2: type eth netdev eth7 controller 0 flavour
>> >> > > pcivf pfnum 0 vfnum 1 splittable false
>> >> > >   function:
>> >> > >     hw_addr 00:00:00:00:00:00
>> >> > >
>> >> > > $ devlink port show -jp pci/0000:00:08.0/2 {
>> >> > >     "port": {
>> >> > >         "pci/0000:00:08.0/1": {
>> >> > >             "type": "eth",
>> >> > >             "netdev": "eth7",
>> >> > >             "controller": 0,
>> >> > >             "flavour": "pcivf",
>> >> > >             "pfnum": 0,
>> >> > >             "vfnum": 1,
>> >> > >             "splittable": false,
>> >> > >             "function": {
>> >> > >                 "hw_addr": "00:00:00:00:00:00"
>> >> > >             }
>> >> > >         }
>> >> > >     }
>> >> > > }
>> >> > >
>> >> > > From earlier email:
>> >> > >
>> >> > > pci/0000:00:08.0/1: type eth netdev eth6 flavour pcipf pfnum 0
>> >> > > pci/0000:00:08.0/2: type eth netdev eth7 flavour pcipf pfnum 0
>> >> > >
>> >> > > If you never use pfnum, you can just put the controller ID there,
>> >> > > like
>> >> Netronome.
>> >> > >
>> >> > It likely not going to work for us. Because pfnum is not some
>> >> > randomly
>> >> generated number.
>> >> > It is linked to the underlying PCI pf number. {pf0, pf1...}
>> >> > Orchestration sw uses this to identify representor of a PF-VF pair.
>> >>
>> >> For orchestration software which is unaware of controllers ports will
>> >> still alias on pf/vf nums.
>> >>
>> >Yes.
>> >Orchestration which will be aware of controller, will use it.
>> >
>> >> Besides you have one devlink instance per port currently so I'm
>> >> guessing there is no pf1 ever, in your case...
>> >>
>> >Currently there are multiple devlink instance. One for pf0, other for pf1.
>> >Ports of both instances have the same switch id.
>> >
>> >> > Replacing pfnum with controller number breaks this; and it still
>> >> > doesn't tell user
>> >> that it's the pf on other_host.
>> >>
>> >> Neither does the opaque controller id.
>> >Which is why I tossed the epcipf (external pci pf) port flavour that fits in
>> current model.
>> >But doesn't allow multiple external hosts under same eswitch for those
>> devices which has same pci pf, vf numbers among those hosts. (and it is the
>> case for mlnx).
>> >
>> >> Maybe now you understand better why I wanted peer objects :/
>> >>
>> >I wasn't against peer object. But showing netdev of peer object assumed
>> no_smartnic, it also assume other_side is also similar Linux kernel.
>> >Anyways, I make humble request get over the past to move forward. :-)
>> >
>> >> > So it is used, and would like to continue to use even if there are
>> >> > multiple PFs
>> >> port (that has same pfnum) under the same eswitch.
>> >> >
>> >> > In an alternative,
>> >> > Currently we have pcipf, pcivf (and pcisf) flavours. May be if we
>> >> > introduce new
>> >> flavour say 'epcipf' to indicate external pci PF/VF/SF ports?
>> >> > There can be better name than epcipf. I just put epcipf to differentiate
>> it.
>> >> > However these ports have same attributes as pcipf, pcivf, pcisf flavours.
>> >>
>> >> I don't think the controllers are a terrible idea. Seems like a
>> >> fairly reasonable extension.
>> >Ok.
>> >> But MLX don't seem to need them. And you have a history of trying to
>> >> make the Linux APIs look like your FW API.
>> >>
>> >Because there are two devlink instances for each PF?
>> >I think for now an epcipf, epcivf flavour would just suffice due to lack of
>> multiple devlink instances.
>> >But in long run it is better to have the controller covering few topologies.
>> >Otherwise we will break the rep naming later when multiple controllers are
>> managed by single eswitch (without notion of controller).
>> >
>> >Sometime my text is confusing. :-) so adding example of the thoughts
>> below.
>> >Example: Eswitch side devlink port show for multi-host setup considering
>> the smartnic.
>> >
>> >$ devlink port show
>> >pci/0000:00:08.0/0: type eth netdev enp0s8f0 flavour physical
>> >pci/0000:00:08.0/1: type eth netdev enp0s8f0_pf0 flavour pcipf pfnum 0
>> >pci/0000:00:08.0/2: type eth netdev enp0s8f0_c0pf0 flavour epcipf pfnum 0
>> >                                                                                                             ^^^^^ new port
>> flavour.
>> >pci/0000:00:08.1/0: type eth netdev enp0s8f1 flavour physical
>> >pci/0000:00:08.1/1: type eth netdev enp0s8f1_pf1 flavour pcipf pfnum 1
>> >pci/0000:00:08.1/2: type eth netdev enp0s8f1_c0pf1 flavour epcipf pfnum
>> >1
>> >
>> >Here one controller has two pci pfs (0,1}. Eswitch shows that they are
>> external pci ports.
>> >Whenever (not sure when), mlnx converts to single devlink instance, this
>> will continue to work.
>> >It will also work when multiple controller(s) (of external host) ports have
>> same switch_id (for orchestration).
>> >And this doesn't break any backward compatibility for non multihost, non
>> smatnic users.
>> >
>> >> Jiri, would you mind chiming in? What's your take?
>> >
>> >Will wait for his inputs..
>> 
>> I don't see the need for new flavour. The port is still pf same as the local pf, it
>> only resides on a different host. We just need to make sure to resolve the
>> conflict between PFX and PFX on 2 different hosts (local/ext or ext/ext)
>> 
>Yes. I agree. I do not have strong opinion on new flavour as long as we make clear that this is for the external controller.
>
>> So I think that for local PFs, no change is needed.
>Yep.
>
>> The external PFs need to have an extra attribute with "external
>> enumeration" what would be used for the representor netdev name as well.
>> 
>> pci/0000:00:08.0/0: type eth netdev enp0s8f0 flavour physical
>> pci/0000:00:08.0/1: type eth netdev enp0s8f0_pf0 flavour pcipf pfnum 0
>> pci/0000:00:08.0/2: type eth netdev enp0s8f0_e0pf0 flavour pcipf extnum 0
>> pfnum 0
>
>How about a prefix of "ec" instead of "e", like?
>pci/0000:00:08.0/2: type eth netdev enp0s8f0_ec0pf0 flavour pcipf ecnum 0 pfnum 0

Yeah, looks fine to me. Jakub?


>                                                                                     ^^^^
>> pci/0000:00:08.0/3: type eth netdev enp0s8f0_e1pf0 flavour pcipf extnum 1
>> pfnum 0
>

  reply	other threads:[~2020-09-01  9:17 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-25 13:58 [PATCH net-next 0/3] devlink show controller info Parav Pandit
2020-08-25 13:58 ` [PATCH net-next 1/3] devlink: Add comment block for missing port attributes Parav Pandit
2020-08-25 13:58 ` [PATCH net-next 2/3] devlink: Consider other controller while building phys_port_name Parav Pandit
2020-08-26  0:32   ` Jakub Kicinski
2020-08-26  4:27     ` Parav Pandit
2020-08-26 20:07       ` Jakub Kicinski
2020-08-27  4:31         ` Parav Pandit
2020-08-27 18:32           ` Jakub Kicinski
2020-08-27 20:15             ` Parav Pandit
2020-08-27 21:42               ` Jakub Kicinski
2020-08-28  4:27                 ` Parav Pandit
2020-08-28  5:08                   ` Parav Pandit
2020-08-28 16:43                   ` Jakub Kicinski
2020-08-29  3:43                     ` Parav Pandit
2020-09-01  8:19                       ` Jiri Pirko
2020-09-01  8:53                         ` Parav Pandit
2020-09-01  9:17                           ` Jiri Pirko [this message]
2020-09-01 21:28                             ` Jakub Kicinski
2020-09-02  4:26                               ` Parav Pandit
2020-09-02  4:44                                 ` Parav Pandit
2020-09-02  8:00                                 ` Jiri Pirko
2020-09-02 15:23                                   ` Jakub Kicinski
2020-09-02 16:18                                     ` Parav Pandit
2020-09-02 20:10                                       ` Parav Pandit
2020-09-03  5:54                                     ` Jiri Pirko
2020-09-03 19:31                                       ` Jakub Kicinski
2020-09-04  8:43                                         ` Jiri Pirko
2020-09-06  3:08                                           ` Parav Pandit
2020-09-06 16:46                                             ` Jakub Kicinski
2020-09-07  7:21                                             ` Jiri Pirko
2020-09-07 16:18                                               ` Jakub Kicinski
2020-08-25 13:58 ` [PATCH net-next 3/3] net/mlx5: E-switch, Set controller attribute for PCI PF and VF ports Parav Pandit
2020-09-08 14:42 ` [PATCH net-next v2 0/6] devlink show controller number Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 1/6] net/mlx5: E-switch, Read controller number from device Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 2/6] devlink: Add comment block for missing port attributes Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 3/6] devlink: Move structure comments outside of structure Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 4/6] devlink: Introduce external controller flag Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 5/6] devlink: Introduce controller number Parav Pandit
2020-09-08 18:50     ` Jakub Kicinski
2020-09-09  3:06       ` Parav Pandit
2020-09-08 14:42   ` [PATCH net-next v2 6/6] devlink: Use controller while building phys_port_name Parav Pandit
2020-09-09  4:50 ` [PATCH net-next v3 0/6] devlink show controller number Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 1/6] net/mlx5: E-switch, Read controller number from device Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 2/6] devlink: Add comment block for missing port attributes Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 3/6] devlink: Move structure comments outside of structure Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 4/6] devlink: Introduce external controller flag Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 5/6] devlink: Introduce controller number Parav Pandit
2020-09-09  4:50   ` [PATCH net-next v3 6/6] devlink: Use controller while building phys_port_name Parav Pandit
2020-09-10 15:02     ` David Ahern
2020-09-09 15:34   ` [PATCH net-next v3 0/6] devlink show controller number Jakub Kicinski
2020-09-09 21:20     ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200901091742.GF3794@nanopsycho.orion \
    --to=jiri@resnulli.us \
    --cc=davem@davemloft.net \
    --cc=jiri@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=parav@mellanox.com \
    --cc=parav@nvidia.com \
    --cc=roid@mellanox.com \
    --cc=saeedm@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).