Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Keller, Jacob E" <jacob.e.keller@intel.com>
To:     Leon Romanovsky <leon@kernel.org>, Jakub Kicinski <kuba@kernel.org>
CC:     ivecera <ivecera@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "sassmann@redhat.com" <sassmann@redhat.com>,
        "Piotrowski, Patryk" <patryk.piotrowski@intel.com>,
        SlawomirX Laba <slawomirx.laba@intel.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net] iavf: Do not restart Tx queues after reset task
 failure
Thread-Topic: [PATCH net] iavf: Do not restart Tx queues after reset task
 failure
Thread-Index: AQHY81xt1U/+TsWFeEauMLnzqEhDea426S2AgAAeHmCAATnmgIAAJckAgAA3HgCAAAwHgIAAASmw
Date:   Thu, 10 Nov 2022 21:13:56 +0000
Message-ID: <CO1PR11MB50894F87FB9571AD4C2FF4A2D6019@CO1PR11MB5089.namprd11.prod.outlook.com>
References: <20221108102502.2147389-1-ivecera@redhat.com>
 <Y2vvbwkvAIOdtZaA@unreal>
 <CO1PR11MB508996B0D00B5FE6187AF085D63E9@CO1PR11MB5089.namprd11.prod.outlook.com>
 <20221110155147.1a2c57f6@p1.luc.cera.cz> <Y20vtqd6raqg8iwy@unreal>
 <20221110122418.32414666@kernel.org> <Y21oCUjEHEMr9HGb@unreal>
In-Reply-To: <Y21oCUjEHEMr9HGb@unreal>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ccfwCCE8OE4UEM30WqO/tbF2O3rl4LwVlKcLGAxWbenMpdQgzHjx6fL1rBAg?=
 =?us-ascii?Q?FGR8lHgDzDh6KZRCEbOCfFz9D0iWH/9pcpyn0J+9WsI/FSCf0JjkW2EmSQT2?=
 =?us-ascii?Q?jmgDD9Q6V6dfBQcp98Ed1fd2UcFQ9VGt14vBXYD+JKgdNcFFQYeI1fw/ZcD4?=
 =?us-ascii?Q?eX+zxad6Ew+jfoBd2plSy0ouZolmvEyXw/NJPKhyUD3h9W7AIjUlQmVSZkH9?=
 =?us-ascii?Q?YGYyRPexM8/5VvVlCamxuqyySXzXiaEuAoUV21M+wISXXO+j9DQWxPWGghAM?=
 =?us-ascii?Q?VYrRAkBdcxFku8+zKyvVayjB3zXeuuKDniDj5kZVsqsh5fAux7erJTlGnGz5?=
 =?us-ascii?Q?YkWCZvKZqERlu4LfQL+RmKyTLTkqZtfYsW+varIlynl6JcxKWF3jlq4KhQEI?=
 =?us-ascii?Q?if2yRX0vhRDCfPxExYpY9QOjAKluvgV9F8Aw/2ll6YNGj5DQau9suCy7uAbz?=
 =?us-ascii?Q?1gMAR+rGmujWj0PQv7LPSKFqIO/FQFgZJLejD4rWDK1RuO4WSAfNKdl/wVYn?=
 =?us-ascii?Q?NhNM1MxC1f8DadKeT2+aJzE7Mmh8BGVCB0EzP15IkwD772EzrJA8/mtS5jMw?=
 =?us-ascii?Q?a17mEACdCtoQCY+SLcZ+S0+WzB3S3VhnUdhwPhuLINFYiM/qbB/1qgxbUYJf?=
 =?us-ascii?Q?oL/IVT5VwQweRIaAxqk2nrkyxbRUEdKrwMkviTIYAZfWj/sRyW1geYIDHVpT?=
 =?us-ascii?Q?ALJq3j6YsNfs+i+lZMJAMLcW8iUOih7fy4HkALo8QOwoNpoXBfqa2tV7JHrw?=
 =?us-ascii?Q?dNdbChZEj75jjDLrs1a3TPOcRUu/rBvVb6UTuZOi2M0OQxBYYL/cJnqPpgMi?=
 =?us-ascii?Q?4srW0+mxvbCb3idNm+clfO8RXWJusygsq5d3L5Px94ey8ZokNArSkcTduTNT?=
 =?us-ascii?Q?nkfnC2k49z86+4/4W2c21IwRc4Fc8Qa3WGHsjTUtziPC66OTJWG9cvLjYWaq?=
 =?us-ascii?Q?MqvLx4k/NqXXXb12TbYf+YP7YyswFQfv/80ldumaMO07alKMjRsescr+tzZ5?=
 =?us-ascii?Q?GzeRtuOp3Cpzgn5Uwz2C7Cf82aXi+Aq8eqwKtA3ZKXB808sKxR78fRFdnitT?=
 =?us-ascii?Q?K+QN9MKhqW5vU3ldlrSxqcx4IiM7uZAlxqNarEnuzLHtbVN00rQHX21bTrbb?=
 =?us-ascii?Q?5EDdjr2+ar3F3SqovIaiAnU41cZ4v5c5GTynl3W+va8c7L6XEe3y1RxHEvMm?=
 =?us-ascii?Q?kBHPjmt7WGnHkMY7KZvTXVF2eO8K93oEfbvDVvoltYGSiG+muUGOaHq8Nwik?=
 =?us-ascii?Q?tInQDKH9n3CnsBZBHVMfySPZT+gAAolt7UWGqRSTUfrCbUBtNSg+i60zlNAK?=
 =?us-ascii?Q?fT82SpNWoCW9xkUedwO64VuQVCzIawT4TlmrtaqPq7tbR5oJujzALuzSWwfc?=
 =?us-ascii?Q?S/x263Z+8nCHAPU6vQIpX5bYXYjLtSZXqI1XWaEsynB0JK7XI/rtBymSqxbT?=
 =?us-ascii?Q?Xv7DbBSKwcOeARuErwc3foHvWFB5jGz0NO0QQP5yk73b91RkWHjkNZkd5H2K?=
 =?us-ascii?Q?2bSDmklCi+DKPKjHXiPDPRkMNQZwNAeza4B56AJuUfiQEO+lHKuqbVy4xFWz?=
 =?us-ascii?Q?uArWsZIqolYYk6ZyNkg2VSWMEpiFHil1RGB2cPB+?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8e7abdfa-6fa3-4048-ee82-08dac3607a85
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Nov 2022 21:13:56.5752
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: JeG4ae9iwmZXy6RFO38awoLiXk8O/lE0tW12zL/+3lOO8fzgGZAuaHw9QEBDAOMUUlE7UyX/wJSYW2xoBYvJHigtUvgWhQorbV6xrsnLgSw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5435
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Thursday, November 10, 2022 1:07 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: ivecera <ivecera@redhat.com>; Keller, Jacob E <jacob.e.keller@intel.c=
om>;
> netdev@vger.kernel.org; sassmann@redhat.com; Piotrowski, Patryk
> <patryk.piotrowski@intel.com>; SlawomirX Laba <slawomirx.laba@intel.com>;
> Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; intel-
> wired-lan@lists.osuosl.org; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH net] iavf: Do not restart Tx queues after reset task =
failure
>=20
> On Thu, Nov 10, 2022 at 12:24:18PM -0800, Jakub Kicinski wrote:
> > On Thu, 10 Nov 2022 19:07:02 +0200 Leon Romanovsky wrote:
> > > > > Yes I think you're right. A ton of people check it without the
> > > > > lock but I think thats not strictly safe. Is dev_close safe to
> > > > > call when netif_running is false? Why not just remove the check
> > > > > and always call dev_close then.
> > > >
> > > > Check for a bit value (like netif_runnning()) is much cheaper than
> > > > unconditionally taking global lock like RTNL.
> > >
> > > This cheap operation is racy and performed in non-performance
> > > critical path.
> >
> > To be clear - the rtnl_lock around the entire if is still racy
> > in the grand scheme of things, no? What's stopping someone from
> > bringing the device right back up after you drop the lock?
>=20

I think the reset flow uses netif_device_detach() to detach the device befo=
re reset. Is that enough to prevent other calls to dev_close outside the dr=
iver?

Also, perhaps we should avoid re-attaching the device if the reset fails...

> I want to believe what there is some sort of state machine that won't
> allow simple toggling of dev_close/dev_open. If it doesn't, rtnl_lock
> users should audit their code for possible toggling right after that
> lock is dropped.
>=20

I think the key is that normally dev_open and dev_close are done by iproute=
2 netlink messages? so if we close it, its possible userspace could immedia=
tely open it.. though I think that isn't allowed while the device is detach=
ed, so we should stay closed until we re-attach, at which point dev_open ca=
n fail by noticing the VF is disabled...


> Anyway, this discussion reminds me our devl_lock debate where we had
> completely opposite views if rtnl_lock model is the right one.
> https://lore.kernel.org/netdev/20211101073259.33406da3@kicinski-fedora-
> PC1C0HJN/
>=20
> Let's not start argue again, we had enough back then. :)
>=20
> Thanks