Received-SPF: pass (google.com: domain of linux-kernel+bounces-213047-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <514caa65-5794-4f1f-9f8f-d11029460c5f@intel.com>
Date: Thu, 13 Jun 2024 12:44:28 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [Intel-wired-lan] [PATCH iwl-net 0/3] ice: fix synchronization
 between .ndo_bpf() and reset
To: Larysa Zaremba <larysa.zaremba@intel.com>, Jakub Kicinski
	<kuba@kernel.org>
CC: Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Jesper Dangaard Brouer
	<hawk@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
	<netdev@vger.kernel.org>, John Fastabend <john.fastabend@gmail.com>, "Alexei
 Starovoitov" <ast@kernel.org>, Eric Dumazet <edumazet@google.com>, "Michal
 Kubiak" <michal.kubiak@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>,
	<magnus.karlsson@intel.com>, <intel-wired-lan@lists.osuosl.org>,
	<bpf@vger.kernel.org>, Paolo Abeni <pabeni@redhat.com>, "David S. Miller"
	<davem@davemloft.net>, <linux-kernel@vger.kernel.org>
References: <20240610153716.31493-1-larysa.zaremba@intel.com>
 <20240611193837.4ffb2401@kernel.org>
 <ZmlGppe04yuGHvPx@lzaremba-mobl.ger.corp.intel.com>
 <20240612140935.54981c49@kernel.org>
 <ZmqztPo6UDIC6gKx@lzaremba-mobl.ger.corp.intel.com>
Content-Language: en-US
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <ZmqztPo6UDIC6gKx@lzaremba-mobl.ger.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UXpoWXg3Rkt4R3RaSmhlRXRqb2hXRlNyM3hnSHVneEF0WXNjWmJGZnRUSjM3?=
 =?utf-8?B?V1cxTS9jdGwzY0N4aDMwNHRaY3V5UHA0QXE1Nk05R0dEOER0M3BXR2g3bjYr?=
 =?utf-8?B?b1FndS9aMEdmTUE1QUZndVBNL2w4bFhOSXFBL3M2SThoYVVzZHk0NEEwbmRW?=
 =?utf-8?B?WDhveVI5dURiUTJnTDdlMUZCLzFVMm5TVWxpWkJxRTRoYTdBTWUwT1ZaMUh6?=
 =?utf-8?B?WHJ0Q1VoNFJoWE5KVzIzYjFJU3ZjVzM5dkdvemx6MGtmMnZOQUkxWlFmQzlS?=
 =?utf-8?B?djh0K3djOTJaVDFnR0ZYSVVUWVh2UjJKelhkZWFNMEV3WDk2VGYvZG5YdlBL?=
 =?utf-8?B?dHVVTFBLNmJyVkN6NkVmYnpyaXpJZlZrZFJjRUdYVTJHU0YvZUdWRzc2Yk9T?=
 =?utf-8?B?MThvK2hydzRmYXRxYWF0OEJxRVpYeTVHUXJWYXp2cjRXQnlGYWdqUWRVOFVi?=
 =?utf-8?B?NFZOQWMrN0dVM1JSTS9VR1FrL0tZUjUxV1JRRkpIbjlDdWF0UnUzNTl1bjBY?=
 =?utf-8?B?RzRLN1d3bnlCQnBVVmVyMkE4S3J1OHV3WmxUdFNDM2M0V0tvYUlDL2pTcHRH?=
 =?utf-8?B?QWZRSWVOZHkyT3FaRE1RNWpxNzlLVCt5V0FLYjczVTV4WWRQcy9PZVU1SjFH?=
 =?utf-8?B?NHVpZXhGZjR3cDhmVmNrVHVqS0QvSklFdEVRNDJ3d1A1K2tRQWl3TmdBTExD?=
 =?utf-8?B?TEVrM09qSVRWZi9KbVV3TGFpTG05UjZITGgzZTJMaVJyVjB4cW5rRzgzK2o2?=
 =?utf-8?B?WkMvcUMrZSsyUGVxcXpLMEFhL1Ntemp2emxRTjJQSUlrV2ZBZXEyQmR2YTNQ?=
 =?utf-8?B?Y2thNlBPWXlQOUNlOXVoZ0h0MldPdkVZOWpVRWFtcXJUblg3aEkzUi84VUdC?=
 =?utf-8?B?Q0RIdHVGdDcvWlg1ZUg5aDRET0Z6WG5iZ1R4L1FZdXo5elUrNkpWeXZZZFRU?=
 =?utf-8?B?K0tVQnpNaFZjODlWcWVMSkZsRlJIclBFQVhweTJhS0FKVThrK0VhTXNYWWZN?=
 =?utf-8?B?WEkrRDNwYWJQdXoxcGwwejl5YWtmWW5XV1Axam14QnpNN056OGN3OVozUFpq?=
 =?utf-8?B?U3BjR1hlK0F3eGJja1M4OGNsT2x1WlhSem5RMjdPSmtsSlBpbUdZcHZpekU2?=
 =?utf-8?B?QjdtSGJmWkkzZTZ5RnhIOTVLSE9kMGh1NXJoaWRCRW9udXJlNkZtZ1JzQkM5?=
 =?utf-8?B?MFVWOTdlbjhOeHVxdHc3MGV0NzFIcmJabjVXZWg2TUpIc29icWJkTnEyTndn?=
 =?utf-8?B?UGJuNkU0UWdtZEMzblQ5S3FvTXpOUy9ZQ1RoSWFMTFZPUmlsZFIrVW1kQVVZ?=
 =?utf-8?B?bmtqOFFWNjExOVU4cW9FdmI4QUp5VVJRTWltUWRYQkppaVpzUGd6STdmeFFI?=
 =?utf-8?B?ZUJrV2lUVUR6N1lRcCtaOXZJZXZtS2hnaUlzQWpyT1FUY2dhbzZQaSs3TTBw?=
 =?utf-8?B?QXBhK2NyelJtR1EyS3Vubm8rU0o4Tk5TdHR3aTR2ZkhXaFMrSGhma0JVdWZN?=
 =?utf-8?B?SHZTYWs4VjVqUVJhMjYrZVpEWXJ4YkpOdTYvVW9QUGpNdE9KbU5wbmQ5ejBp?=
 =?utf-8?B?U3NjV0Y1bWtzcUpUY2JRNEFQb1BWcUErcldyakVPOWJjczY3MlZTOHZBT3JZ?=
 =?utf-8?B?anNIazNkSTRQaWdnUTNQcTk5N0VhYjJ3cTk0NkR6bFQ2cU1GNWJhcXd6ZTUx?=
 =?utf-8?B?SVNzMnc1RExSSktENXJSRjRZYkg5ZFNmSEJ1b0tWL0VNR20ybmROa3ZHYm1w?=
 =?utf-8?B?OUEzSmVVMUFJK08zM1lYdDBmU3dhZHJuODZNN1VyRHJEV3F4UkNVdWFkUVN1?=
 =?utf-8?B?TmtTZHNTei9iZDBGRklGMURrUVRrNTVFRDhFMnQ1R2JKK0ErSzY4dHBhNm5r?=
 =?utf-8?B?MDBVNzV6L2lwdFVmUDcvdS9Wd29teVJWdnk1QkJHQkVPNVUzaVZ5NktNaHA5?=
 =?utf-8?B?d0cyZVZTTjI2TDZLY1FVV25OYU45eFE3U00wQUtYR00xTy9JSE9yaG5wRUts?=
 =?utf-8?B?NHI5a1BZUFM1QUsxMnk5OExJU0JYYUZPTjR5QnBJTG82OU1MaWhCUGlTMlBy?=
 =?utf-8?B?bmJOcnZyZnZOUWVVSytBYXl5TkxucW5qR3JZOElKQ1I4VWg2ZDNqN1VmbFlN?=
 =?utf-8?B?UEFRTEdmZi9MRmVhS3p6SHJGVlB0UXZ3MURMM09na0E2ODRIVzFYV1hFZldi?=
 =?utf-8?Q?z4htdZPeXrykSZf06g0j40g=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 97ad158f-0ba5-4970-a297-08dc8b95d125
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 10:44:35.5887
 (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: FguWNVrvTBSivj34+R/jusV8Cw0w6Kp4gvoPQmQ5UIyGev0eAqWFlAazrqu4NoHAxU4cpuKSpvhipY2C1WNCvShsbHime7m2OXbTsFWCrC0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5282
X-OriginatorOrg: intel.com

On 6/13/24 10:54, Larysa Zaremba wrote:
> On Wed, Jun 12, 2024 at 02:09:35PM -0700, Jakub Kicinski wrote:
>> On Wed, 12 Jun 2024 08:56:38 +0200 Larysa Zaremba wrote:
>>> On Tue, Jun 11, 2024 at 07:38:37PM -0700, Jakub Kicinski wrote:
>>>> On Mon, 10 Jun 2024 17:37:12 +0200 Larysa Zaremba wrote:
>>>>> Fix the problems that are triggered by tx_timeout and ice_xdp() calls,
>>>>> including both pool and program operations.
>>>>
>>>> Is there really no way for ice to fix the locking? :(
>>>> The busy loops and trylocks() are not great, and seem like duct tape.
>>>
>>> The locking mechanisms I use here do not look pretty, but if I am not missing
>>> anything, the synchronization they provide must be robust.
>>
>> Robust as in they may be correct here, but you lose lockdep and all
>> other infra normal mutex would give you.
>>
> 
> I know, but __netif_queue_set_napi() requires rtnl_lock() inside the potential
> critical section and creates a deadlock this way. However, after reading
> patches that introduce this function, I think it is called too early in the
> configuration. Seems like it should be called somewhere right after
> netif_set_real_num_rx/_tx_queues(), much later in the configuration where we
> already hold the rtnl_lock(). In such way, ice_vsi_rebuild() could be protected
> with an internal mutex. WDYT?
> 
>>> A prettier way of protecting the same critical sections would be replacing
>>> ICE_CFG_BUSY around ice_vsi_rebuild() with rtnl_lock(), this would eliminate
>>> locking code from .ndo_bpf() altogether, ice_rebuild_pending() logic will have
>>> to stay.
>>>
>>> At some point I have decided to avoid using rtnl_lock(), if I do not have to. I
>>> think this is a goal worth pursuing?
>>
>> Is the reset for failure recovery, rather than reconfiguration?
>> If so netif_device_detach() is generally the best way of avoiding
>> getting called (I think I mentioned it to someone @intal recently).

for the reference, it was to Dawid here:
https://lore.kernel.org/netdev/20240610194756.5be5be90@kernel.org/

> 
> AFAIK, netif_device_detach() does not affect .ndo_bpf() calls. We were trying
> such approach with idpf and it does work for ethtool, but not for XDP.