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;
Message-ID: <e15d588e-b63f-ab70-f6ae-91ceea8be79a@intel.com>
Date:   Mon, 3 Apr 2023 10:31:23 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.1
Subject: Re: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     <jgg@nvidia.com>, <yishaih@nvidia.com>,
        <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
        <tglx@linutronix.de>, <darwi@linutronix.de>, <kvm@vger.kernel.org>,
        <dave.jiang@intel.com>, <jing2.liu@intel.com>,
        <ashok.raj@intel.com>, <fenghua.yu@intel.com>,
        <tom.zanussi@linux.intel.com>, <linux-kernel@vger.kernel.org>
References: <cover.1680038771.git.reinette.chatre@intel.com>
 <419f3ba2f732154d8ae079b3deb02d0fdbe3e258.1680038771.git.reinette.chatre@intel.com>
 <20230330164050.0069e2a5.alex.williamson@redhat.com>
 <20230330164214.67ccbdfa.alex.williamson@redhat.com>
 <688393bf-445c-15c5-e84d-1c16261a4197@intel.com>
 <20230331162456.3f52b9e3.alex.williamson@redhat.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230331162456.3f52b9e3.alex.williamson@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WDh6MCtPNlRCejVTemV6OHJpL3d5K2RjWjVSTFNISUZCRWZhMUV2Ui96b1g0?=
 =?utf-8?B?NXd6TmxyS2dtZk8xN3M3TkNXZ29QSHpYV09BWDhkZHYwckUraWZCc1FxVmZn?=
 =?utf-8?B?MjdUeml3YmJvOGxqczhQL2hEVWxtbjl1L1BxWks3dmh5ay9hck5tZDNYVWU1?=
 =?utf-8?B?UlB0RDEvMkdDUy9BUzVqTmZoZ3RaVks0N1UvOFJTdXl1Rmg1VzZ4VjV0MnR0?=
 =?utf-8?B?TGpTRy9LaTdQOHB1azJzMit2NWNXNXcxNHdmc2R5RlF6UllYY3puU2JPem1o?=
 =?utf-8?B?bWlURHdsd25KYXlxZEVwbFJmU01sM0tIblgxMUlQejNYbDh0R05UZkNUR2Q3?=
 =?utf-8?B?MUd0bDlVNjZ0V25rTUYrR1I0OHR6ckNDSllESHlqQVV3SU9xNnZSbjhRQWZs?=
 =?utf-8?B?aG5WNUJTTkxTckZoZzIzdy84a1o4UWtiYm03VXgreW9ZR3MwdjFaVE1VMExV?=
 =?utf-8?B?K0FOSlphS1FIYjNLYm92TnlPcEM2YnUwUk9kMGpIdFZjRDh6dFQxdzRLMXZ0?=
 =?utf-8?B?SGtWeGU1YmxXUkZISis2dFNDNEdqSUh5YlNKUU82TXBmakQwV2Zic1ozOHpa?=
 =?utf-8?B?Z1V0SkhlaUxWQkNIUE5kalo3ejlQbWhNOFBKcmU5bWhHczhiYm0xaVZMQTJn?=
 =?utf-8?B?cWp4Rk1HYkx4SytpT2hvamZGbWFPMU5lay9hK3VSL3ljK0gzQW1xTDI0U2t3?=
 =?utf-8?B?TmRLcXlHdjZTYmFxamUydEtoblJqMms1cno2OHhsN2psMmNRc2d6MFQxWTR1?=
 =?utf-8?B?ZkppODFHMW5RdGVsV2pwYXlNdUw2d09qOFpyMnpkWHZsa1ZVVUVqL2RwRGlm?=
 =?utf-8?B?WWpWVm5aanRNL1NlYnlsSVR4SDBkM25vbWFyMXZnWTJyZWI4elFrS1pVbXdC?=
 =?utf-8?B?cW1uQ0I2MXZPNURhbEM0TVc3ZzlEc2RHQ3VwRUtJdFNnR2Y3Mk01SmZVWnB4?=
 =?utf-8?B?YjFzU2JTSTRlRENNSkk3NjBDcytVVGJ1eFFWU2xQY3Z1Uk1CajhIMHVLUjZs?=
 =?utf-8?B?TEt4N3F5NXN5VloxbWt4TmM1c3ZUNnE4MlN5Q0NaMUg5TjZaMmtPQVd6MlYv?=
 =?utf-8?B?WUpkRUJuL0F3Y1BBaW5zSkJHeWxUb0M5R3FxVDVXSGJpdnhVcTBMNnhIQ3Vu?=
 =?utf-8?B?cTFJMm5Wc1ZEU3hRWU52cUEzc05aN1IyOURVS1c1OFRXSHM5SnNHNFdjMTFD?=
 =?utf-8?B?K3F2SUp6bTUwU0E2NVBCMkxXUWZaK3graDc3U2cwRXNtYUpxVDFybldQL1Jt?=
 =?utf-8?B?S1JPNWJNTHBqNkdMWUI0M3ltcmt4a25FandMZTBSRHJGQkFXV1ZOUDZ5b3Uz?=
 =?utf-8?B?NHlTa3ByekFOUzFsZnFHVkp0SnZRQ3BPTHFYZ0JFaE5sanlxS3g5ZWFYRGJu?=
 =?utf-8?B?S3JoRC9mdEVocWlPb05tQlhWaFdrYlphRUtvNUxaZHdUZm03NjBMdCtSMnB5?=
 =?utf-8?B?YWlYZHl3Y2Q5UHNQTTRDZkRqcTJqZllHMDVKN3gzY3owWUJKTGUrditHbEo1?=
 =?utf-8?B?M0RBT216REVlUUg3VkZheUNLZVBtSDJGSnlheXRGWVh0c2FNZ0p4SWRkT2kx?=
 =?utf-8?B?a0s3N2xEZWd0b21LUnVxT0NCSTJEZGpxcUlEQmNJV3RzNDVpOEZEejlid3Y5?=
 =?utf-8?B?cklqMXRpTkNzVzlrak91YVpibGVWUkJmOE8wMFdqbWl6M3JhV2F0d3h3S0x0?=
 =?utf-8?B?bjNCV3NpY1gvZFMrdVpMcnJmZGdLVGpCTG53TzFrNHlnZlNkTzk4WWd6b25L?=
 =?utf-8?B?Wlg5eklOWDRwU1N0MFY3dkcvNHphQWpLYm1zdFVZejAydDVkSU1HeDVkWUh6?=
 =?utf-8?B?UlNsTUUxSU8xZi9QWHdlOUMxVEo2UnkvVGNBUERla2ZvSzVXd3N6NWNNNERt?=
 =?utf-8?B?YkY5UWswU0hSZmkzdEY3T252NmdSZTlITnBNVDBXV1g0ZE5xODNmbUs5WnNr?=
 =?utf-8?B?MXEyMmlCMGNxS2dKbFN3M1Y5dDd2bEd5cHJ2MmFsVW9zbW96Z2JpTWlXWnNN?=
 =?utf-8?B?YUVJYWpvcTlDUXoyckt3ZjJBREVUSmszcHBUY1VLYlUrTzVWN0VIOHg3blpr?=
 =?utf-8?B?QlBRekF6RXVVSUNDNDNmdHNJNlErL1dJaXRlTDRYQ1p1RGR5UVFoQjU3clFj?=
 =?utf-8?B?V3lDUk1kOVBxZ0tuS0xGbDhyb1hKQ29vM0Vna3l2c2RXZkdBdkZNUXJnT211?=
 =?utf-8?B?dVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4c5e43a5-6259-4522-1b0d-08db34694107
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2023 17:31:27.3036
 (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: baUhLm/K1D++9x9OczlySs4x1hHxZrZa2kIRm2INDsSfIfc/5JhbkpznEGmM7jLROxucOYB3ghZ7uIRAygP5pXmeNc5irba4hdWjj0ukACk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6535
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.8 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable 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

Hi Alex,

On 3/31/2023 3:24 PM, Alex Williamson wrote:
> On Fri, 31 Mar 2023 10:49:16 -0700
> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 3/30/2023 3:42 PM, Alex Williamson wrote:
>>> On Thu, 30 Mar 2023 16:40:50 -0600
>>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>>   
>>>> On Tue, 28 Mar 2023 14:53:34 -0700
>>>> Reinette Chatre <reinette.chatre@intel.com> wrote:
>>>>  

...

>>>>> +		msix_map.index = vector;
>>>>> +		msix_map.virq = irq;
>>>>> +		pci_msix_free_irq(pdev, msix_map);
>>>>> +	}
>>>>> +	vfio_pci_memory_unlock_and_restore(vdev, cmd);
>>>>>  out_put_eventfd_ctx:
>>>>>  	eventfd_ctx_put(trigger);
>>>>>  out_free_name:
>>>>>  	kfree(ctx->name);
>>>>>  	ctx->name = NULL;
>>>>> +out_free_ctx:
>>>>> +	if (allow_dyn_alloc && new_ctx)
>>>>> +		vfio_irq_ctx_free(vdev, ctx, vector);
>>>>>  	return ret;
>>>>>  }
>>>>>      
>>>>
>>>> Do we really need the new_ctx test in the above cases?  Thanks,  
>>
>> new_ctx is not required for correctness but instead is used to keep
>> the code symmetric. 
>> Specifically, if the user enables MSI-X without providing triggers and
>> then later assign triggers then an error path without new_ctx would unwind
>> more than done in this function, it would free the context that
>> was allocated within vfio_msi_enable(). 
> 
> Seems like we already have that asymmetry, if a trigger is unset we'll
> free the ctx allocated by vfio_msi_enable().  Tracking which are

Apologies, but could you please elaborate on where the asymmetry is? I am
not able to see a flow in this solution where the ctx allocated by
vfio_msi_enable() is freed if the trigger is unset.

> allocated where is unnecessarily complex, how about a policy that

I do not see this as tracking where allocations are made. Instead I
see it as containing/compartmentalizing state changes with the goal of
making the code easier to understand and maintain. Specifically, new_ctx
is used so that if vfio_msi_set_vector_signal() fails, the state 
before and after vfio_msi_set_vector_signal() will be the same.

I do agree that it makes vfio_msi_set_vector_signal() more complex
and I can remove new_ctx if you find that this is unnecessary after
considering the motivations behind its use. 

> devices supporting vdev->has_dyn_msix only ever have active contexts
> allocated?  Thanks,

What do you see as an "active context"? A policy that is currently enforced
is that an allocated context always has an allocated interrupt associated
with it. I do not see how this could be expanded to also require an
enabled interrupt because interrupt enabling requires a trigger that
may not be available.

Reinette