From: Jason Wang <jasowang@redhat.com>
To: Heng Qi <hengqi@linux.alibaba.com>
Cc: netdev@vger.kernel.org, virtualization@lists.linux.dev,
"Michael S. Tsirkin" <mst@redhat.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Eric Dumazet <edumazet@google.com>,
"David S. Miller" <davem@davemloft.net>,
Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Subject: Re: [PATCH 2/2] virtio-net: reduce the CPU consumption of dim worker
Date: Tue, 26 Mar 2024 14:05:10 +0800 [thread overview]
Message-ID: <CACGkMEusLdoKA7pBWnKCLR5yEJ4JN-ivEJquEgKjh3pfKSta8A@mail.gmail.com> (raw)
In-Reply-To: <75f1ae35-aeee-404a-be1c-2ffa05126cdb@linux.alibaba.com>
On Tue, Mar 26, 2024 at 1:57 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
>
> 在 2024/3/26 下午12:08, Jason Wang 写道:
> > On Tue, Mar 26, 2024 at 10:46 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>
> >>
> >> 在 2024/3/25 下午4:42, Jason Wang 写道:
> >>> On Mon, Mar 25, 2024 at 4:22 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>>>
> >>>> 在 2024/3/25 下午3:56, Jason Wang 写道:
> >>>>> On Mon, Mar 25, 2024 at 3:18 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>>>>> 在 2024/3/25 下午1:57, Jason Wang 写道:
> >>>>>>> On Mon, Mar 25, 2024 at 10:21 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>>>>>>> 在 2024/3/22 下午1:19, Jason Wang 写道:
> >>>>>>>>> On Thu, Mar 21, 2024 at 7:46 PM Heng Qi <hengqi@linux.alibaba.com> 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@linux.alibaba.com>
> >>>>>>>>> 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@redhat.com/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@redhat.com/t/
> >> [2] https://lore.kernel.org/all/20221226074908.8154-1-jasowang@redhat.com/
> >> [3] https://lore.kernel.org/all/20230413064027.13267-1-jasowang@redhat.com/
> >> [4] https://lore.kernel.org/all/20230524081842.3060-1-jasowang@redhat.com/
> >> [5] https://lore.kernel.org/all/20230720083839.481487-1-jasowang@redhat.com/
> >>
> >> 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@kernel.org/
> > 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.
>
> OK, that puts my concerns to rest.
>
> >
> >> 2. Adding the interrupt for ctrlq may break some devices
> >> https://lore.kernel.org/all/f9e75ce5-e6df-d1be-201b-7d0f18c1b6e7@redhat.com/
> > These devices need to be fixed. It's hard to imagine the evolution of
> > virtio-net is blocked by buggy devices.
>
> Agree.
>
> >
> >> 3. RTNL breaks surprise removal
> >> https://lore.kernel.org/all/20230720170001-mutt-send-email-mst@kernel.org/
> > 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.
>
> Then I will make a patch set based on irq and without timeout.
Ok.
>
> >
> >> 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@nvidia.com/
> > I don't see how it conflicts with this.
>
> I'll just make changes on top of it. Can I?
It should be fine.
Thanks
>
> Thanks,
> Heng
>
> >
> > 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
>
next prev parent reply other threads:[~2024-03-26 6:05 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 11:45 [PATCH 0/2] virtio-net: a fix and some updates for virtio dim Heng Qi
2024-03-21 11:45 ` [PATCH 1/2] virtio-net: fix possible dim status unrecoverable Heng Qi
2024-03-22 5:17 ` Jason Wang
2024-03-25 2:11 ` Heng Qi
2024-03-25 6:29 ` Jason Wang
2024-03-25 6:57 ` Heng Qi
2024-03-25 7:06 ` Jason Wang
2024-03-21 11:45 ` [PATCH 2/2] virtio-net: reduce the CPU consumption of dim worker Heng Qi
2024-03-22 2:03 ` kernel test robot
2024-03-22 5:19 ` Jason Wang
2024-03-25 2:21 ` Heng Qi
2024-03-25 5:57 ` Jason Wang
2024-03-25 7:17 ` Heng Qi
2024-03-25 7:56 ` Jason Wang
2024-03-25 8:22 ` Heng Qi
2024-03-25 8:42 ` Jason Wang
2024-03-26 2:46 ` Heng Qi
2024-03-26 4:08 ` Jason Wang
2024-03-26 5:57 ` Heng Qi
2024-03-26 6:05 ` Jason Wang [this message]
2024-03-22 6:50 ` Dan Carpenter
2024-03-21 12:25 ` [PATCH 0/2] virtio-net: a fix and some updates for virtio dim Jiri Pirko
2024-03-25 2:23 ` Heng Qi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACGkMEusLdoKA7pBWnKCLR5yEJ4JN-ivEJquEgKjh3pfKSta8A@mail.gmail.com \
--to=jasowang@redhat.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hengqi@linux.alibaba.com \
--cc=kuba@kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=virtualization@lists.linux.dev \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).