Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yuval Avnery <yuvalav@mellanox.com>
To:     Jakub Kicinski <jakub.kicinski@netronome.com>
CC:     Jiri Pirko <jiri@mellanox.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
Thread-Topic: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
Thread-Index: AQHVr871nDBB5hrvy0ak2C7+9Ay1lae1Oa8AgAAB1gCAABOZgIAACO8Q
Date:   Wed, 11 Dec 2019 19:57:34 +0000
Message-ID: <AM6PR05MB5142CCAB9A06DAC199F7100CC55A0@AM6PR05MB5142.eurprd05.prod.outlook.com>
References: <1576033133-18845-1-git-send-email-yuvalav@mellanox.com>
        <20191211095854.6cd860f1@cakuba.netronome.com>
        <AM6PR05MB514244DC6D25DDD433C0E238C55A0@AM6PR05MB5142.eurprd05.prod.outlook.com>
 <20191211111537.416bf078@cakuba.netronome.com>
In-Reply-To: <20191211111537.416bf078@cakuba.netronome.com>
Accept-Language: he-IL, en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9cc46e15-47c9-4b62-a095-08d77e745d45
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Dec 2019 19:57:34.1215
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ScFmuPDrPs9MPDXO86jr6gHNty7YmKuG3RQL+j/fiBDmzYnqzL4TYiNdsMfp/btZzp0zjXOFPezsYzyb6S8UVQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR05MB5160
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Wednesday, December 11, 2019 11:16 AM
> To: Yuval Avnery <yuvalav@mellanox.com>
> Cc: Jiri Pirko <jiri@mellanox.com>; davem@davemloft.net;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
>=20
> On Wed, 11 Dec 2019 18:19:56 +0000, Yuval Avnery wrote:
> > > On Wed, 11 Dec 2019 04:58:53 +0200, Yuval Avnery wrote:
> > > > Currently there is no limit to the number of VFs netdevsim can enab=
le.
> > > > In a real systems this value exist and used by driver.
> > > > Fore example, Some features might need to consider this value when
> > > > allocating memory.
> > >
> > > Thanks for the patch!
> > >
> > > Can you shed a little bit more light on where it pops up? Just for my
> curiosity?
> >
> > Yes, like we described in the subdev threads.
> > User should be able to configure some attributes before the VF was
> enabled.
> > So all those (persistent) VF attributes should be available for query
> > and configuration before VF was enabled.
> > The driver can allocate an array according to max_vfs to hold all that
> > data, like we do here in" vfconfigs".
>=20
> I was after more practical reasoning, are you writing some tests for subd=
ev
> stuff that will depend on this change? :)

Yes we are writing tests for subdev with this.
This is the way mlx5 works.. is that practical enough?

>=20
> > > > Signed-off-by: Yuval Avnery <yuvalav@mellanox.com>
> > > > Acked-by: Jiri Pirko <jiri@mellanox.com>
> > > >
> > > > diff --git a/drivers/net/netdevsim/bus.c
> > > > b/drivers/net/netdevsim/bus.c index 6aeed0c600f8..f1a0171080cb
> > > > 100644
> > > > --- a/drivers/net/netdevsim/bus.c
> > > > +++ b/drivers/net/netdevsim/bus.c
> > > > @@ -26,9 +26,9 @@ static struct nsim_bus_dev
> > > > *to_nsim_bus_dev(struct device *dev)  static int
> > > > nsim_bus_dev_vfs_enable(struct nsim_bus_dev
> > > *nsim_bus_dev,
> > > >  				   unsigned int num_vfs)
> > > >  {
> > > > -	nsim_bus_dev->vfconfigs =3D kcalloc(num_vfs,
> > > > -					  sizeof(struct nsim_vf_config),
> > > > -					  GFP_KERNEL);
> > >
> > > You're changing the semantics of the enable/disable as well now.
> > > The old values used to be wiped when SR-IOV is disabled, now they
> > > will be retained across disable/enable pair.
> > >
> > > I think it'd be better if that wasn't the case. Users may expect a
> > > system to be in the same state after they enable SR-IOV, regardless
> > > if someone else used SR-IOV since last reboot.
> >
> > Right,
> > But some values should retain across enable/disable, for example MAC
> address which is persistent.
> > So maybe we need to retain some values, while resetting others on
> disable?
> > Would that work?
>=20
> Mmm. That is a good question. For all practical purposes SR-IOV used to b=
e
> local to the host that enables it until Smart/middle box NICs emerged.
>=20
> Perhaps the best way forward would be to reset the config that was set vi=
a
> legacy APIs and keep only the MACs provisioned via persistent devlink API=
?
>=20
> So for now we'd memset, and once devlink API lands reset selectively?

Legacy is also persistent.
Currently when you set mac address with "ip link vf set mac" it is persiste=
nt (at least in mlx5).
But ip link only exposes enabled VFS, so driver on VF has to reload to acqu=
ire this MAC.
With devlink subdev it will be possible to set the MAC before VF was enable=
d.

I think we need to distinguish here between:
- PF sets MAC to a VF - persistent.
- VF sets MAC to itself - not persistent.

But is the second case relevant in netdevsim?


>=20
> > > Could you add a memset(,0,) here?
> > >
> > > > +	if (nsim_bus_dev->max_vfs < num_vfs)
> > > > +		return -ENOMEM;
> > > > +
> > > >  	if (!nsim_bus_dev->vfconfigs)
> > > >  		return -ENOMEM;
> > >
> > > This check seems useless now, no? We will always have vfconfigs