On Tue, Mar 26, 2024 at 10:46 AM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > > > > 在 2024/3/25 下午4:42, Jason Wang 写道: > > On Mon, Mar 25, 2024 at 4:22 PM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > >> > >> > >> 在 2024/3/25 下午3:56, Jason Wang 写道: > >>> On Mon, Mar 25, 2024 at 3:18 PM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > >>>> > >>>> 在 2024/3/25 下午1:57, Jason Wang 写道: > >>>>> On Mon, Mar 25, 2024 at 10:21 AM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > >>>>>> 在 2024/3/22 下午1:19, Jason Wang 写道: > >>>>>>> On Thu, Mar 21, 2024 at 7:46 PM Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> wrote: > >>>>>>>> Currently, ctrlq processes commands in a synchronous manner, > >>>>>>>> which increases the delay of dim commands when configuring > >>>>>>>> multi-queue VMs, which in turn causes the CPU utilization to > >>>>>>>> increase and interferes with the performance of dim. > >>>>>>>> > >>>>>>>> Therefore we asynchronously process ctlq's dim commands. > >>>>>>>> > >>>>>>>> Signed-off-by: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx> > >>>>>>> I may miss some previous discussions. > >>>>>>> > >>>>>>> But at least the changelog needs to explain why you don't use interrupt. > >>>>>> Will add, but reply here first. > >>>>>> > >>>>>> When upgrading the driver's ctrlq to use interrupt, problems may occur > >>>>>> with some existing devices. > >>>>>> For example, when existing devices are replaced with new drivers, they > >>>>>> may not work. > >>>>>> Or, if the guest OS supported by the new device is replaced by an old > >>>>>> downstream OS product, it will not be usable. > >>>>>> > >>>>>> Although, ctrlq has the same capabilities as IOq in the virtio spec, > >>>>>> this does have historical baggage. > >>>>> I don't think the upstream Linux drivers need to workaround buggy > >>>>> devices. Or it is a good excuse to block configure interrupts. > >>>> Of course I agree. Our DPU devices support ctrlq irq natively, as long > >>>> as the guest os opens irq to ctrlq. > >>>> > >>>> If other products have no problem with this, I would prefer to use irq > >>>> to solve this problem, which is the most essential solution. > >>> Let's do that. > >> Ok, will do. > >> > >> Do you have the link to the patch where you previously modified the > >> control queue for interrupt notifications. > >> I think a new patch could be made on top of it, but I can't seem to find it. > > Something like this? > > YES. Thanks Jason. > > > > > https://lore.kernel.org/lkml/6026e801-6fda-fee9-a69b-d06a80368621@xxxxxxxxxx/t/ > > > > Note that > > > > 1) some patch has been merged > > 2) we probably need to drop the timeout logic as it's another topic > > 3) need to address other comments > > I did a quick read of your patch sets from the previous 5 version: > [1] > https://lore.kernel.org/lkml/6026e801-6fda-fee9-a69b-d06a80368621@xxxxxxxxxx/t/ > [2] https://lore.kernel.org/all/20221226074908.8154-1-jasowang@xxxxxxxxxx/ > [3] https://lore.kernel.org/all/20230413064027.13267-1-jasowang@xxxxxxxxxx/ > [4] https://lore.kernel.org/all/20230524081842.3060-1-jasowang@xxxxxxxxxx/ > [5] https://lore.kernel.org/all/20230720083839.481487-1-jasowang@xxxxxxxxxx/ > > Regarding adding the interrupt to ctrlq, there are a few points where > there is no agreement, > which I summarize below. > > 1. Require additional interrupt vector resource > https://lore.kernel.org/all/20230516165043-mutt-send-email-mst@xxxxxxxxxx/ I don't think one more vector is a big problem. Multiqueue will require much more than this. Even if it is, we can try to share an interrupt as Michael suggests. Let's start from something that is simple, just one more vector. > 2. Adding the interrupt for ctrlq may break some devices > https://lore.kernel.org/all/f9e75ce5-e6df-d1be-201b-7d0f18c1b6e7@xxxxxxxxxx/ These devices need to be fixed. It's hard to imagine the evolution of virtio-net is blocked by buggy devices. > 3. RTNL breaks surprise removal > https://lore.kernel.org/all/20230720170001-mutt-send-email-mst@xxxxxxxxxx/ The comment is for indefinite waiting for ctrl vq which turns out to be another issue. For the removal, we just need to do the wakeup then everything is fine. > > Regarding the above, there seems to be no conclusion yet. > If these problems still exist, I think this patch is good enough and we > can merge it first. I don't think so, poll turns out to be problematic for a lot of cases. > > For the third point, it seems to be being solved by Daniel now [6], but > spink lock is used, > which I think conflicts with the way of adding interrupts to ctrlq. > > [6] https://lore.kernel.org/all/20240325214912.323749-1-danielj@xxxxxxxxxx/ I don't see how it conflicts with this. Thanks > > > Thanks, > Heng > > > > > THanks > > > > > >> Thanks, > >> Heng > >> > >>> Thanks > >>> > >>>>> And I remember you told us your device doesn't have such an issue. > >>>> YES. > >>>> > >>>> Thanks, > >>>> Heng > >>>> > >>>>> Thanks > >>>>> > >>>>>> Thanks, > >>>>>> Heng > >>>>>> > >>>>>>> Thanks >
