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@xxxxxxxxx> wrote: >> On 3/30/2023 3:42 PM, Alex Williamson wrote: >>> On Thu, 30 Mar 2023 16:40:50 -0600 >>> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: >>> >>>> On Tue, 28 Mar 2023 14:53:34 -0700 >>>> Reinette Chatre <reinette.chatre@xxxxxxxxx> 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
