On Mon, 3 Apr 2023 10:31:23 -0700 Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote: > 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. The user first calls SET_IRQS to enable MSI-X with some number of vectors with (potentially) an eventfd for each vector. The user later calls SET_IRQS passing a -1 eventfd for one or more of the vectors with an eventfd initialized in the prior step. Given that we find the ctx, the ctx has a trigger, and assuming dynamic allocation is supported, the ctx is freed and vfio_msi_set_vector_signal() returns w/o allocating a new ctx. We've de-allocated both the irq and context initialized from vfio_msi_enable(). > > 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. That's not really possible given how we teardown the existing ctx before configuring the new one and unwind to disable contexts in vfio_msi_set_block() > 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. If the goal is to allow the user to swap one eventfd for another, where the result will always be the new eventfd on success or the old eventfd on error, I don't see that this code does that, or that we've ever attempted to make such a guarantee. If the ioctl errors, I think the eventfds are generally deconfigured. We certainly have the unwind code that we discussed earlier that deconfigures all the vectors previously touched in the loop (which seems to be another path where we could de-allocate from the set of initial ctxs). > > 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. A context is essentially meant to track a trigger, ie. an eventfd provided by the user. In the static case all the irqs are necessarily pre-allocated, therefore we had no reason to consider a dynamic array for the contexts. However, a given context is really only "active" if it has a trigger, otherwise it's just a placeholder. When the placeholder is filled by an eventfd, the pre-allocated irq is enabled. This proposal seems to be a hybrid approach, pre-allocating some initial set of irqs and contexts and expecting the differentiation to occur only when new vectors are added, though we have some disagreement about this per above. Unfortunately I don't see an API to enable MSI-X without some vectors, so some pre-allocation of irqs seems to be required regardless. But if non-active contexts were only placeholders in the pre-dynamic world and we now manage them via a dynamic array, why is there any pre-allocation of contexts without knowing the nature of the eventfd to fill it? We could have more commonality between cases if contexts are always dynamically allocated, which might simplify differentiation of the has_dyn_msix cases largely to wrappers allocating and freeing irqs. Thanks, Alex
