From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7E9A9C433F5 for ; Wed, 20 Oct 2021 16:34:45 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EC796611EF for ; Wed, 20 Oct 2021 16:34:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org EC796611EF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:34220 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mdEY7-00083I-MB for qemu-devel@archiver.kernel.org; Wed, 20 Oct 2021 12:34:43 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:50676) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mdEWW-0007GH-Ru for qemu-devel@nongnu.org; Wed, 20 Oct 2021 12:33:04 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:40146) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mdEWS-00082s-JG for qemu-devel@nongnu.org; Wed, 20 Oct 2021 12:33:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634747578; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=giK5JKMf4Yqa2+29CFg6zW7AukDEJ/YuSS1yvXMRu4E=; b=C6DQrkCU2gfjzc8kp1Zv+oJF8zcsdPcCu/0OSd7/wvCcnFNM9aE3MDFJwqfL9NueRrxViO WojT3Dkzm9ol9tUgpE27yxI0PECxIOzSVx9gc3A1ikfwc2VnBi/Dr8JUPr5Toq3GIEdfVk ObtkRs83UbxWRjgHA05hCtoQAn+9cgA= Received: from mail-ua1-f69.google.com (mail-ua1-f69.google.com [209.85.222.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-56-JTT7SvDZNyaeT17yyz8JHA-1; Wed, 20 Oct 2021 12:32:57 -0400 X-MC-Unique: JTT7SvDZNyaeT17yyz8JHA-1 Received: by mail-ua1-f69.google.com with SMTP id y31-20020a9f3262000000b002ca2fd8179eso2023143uad.7 for ; Wed, 20 Oct 2021 09:32:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=giK5JKMf4Yqa2+29CFg6zW7AukDEJ/YuSS1yvXMRu4E=; b=Q15cKeKltTnit5IQTYEXnGxPBrFBC+rNwxSbb+lgnqD0k5mEtAnM7ltRQJrzQAsuMJ n9pY76CqiOeZKvjTXOuLB/AY0/Q6YFQszlKmIwr+3v4YQkL9XZN8gTKb09T3/F/1AQjC DRKgOSViaXKkZ+RZ/VzGX5zEAkicOaXqCp/qjnGF96jiz0nCo1cG5BT4i5ZiD1groS4k 51kgrlt8rx4ZuDrobASjegZKN+Bbi42KLuWhmLYfRe824rmgwIo5WhtllTq3LMcPIiji PY/Fq8rFa/0nAhYmB1eO2M8/mf/4kAA/zayy0dMRDQrK8DR/bR0VGuTVirE0gfhHmTAC D3Fw== X-Gm-Message-State: AOAM5308BYCyv7oCVLRTA5ntGmttgS0viIDxzsAvA2aUUnzL1r5lcC2Y PH+TXF2jweTeU+pBP7O8s7sMwwvFRb3uIXTC+EMS3ahDrskKZ4h4ugA7LUHUzm3yGYNoJ++8nvX 8Wf01iD8/qnrXyPCga7tW5lH9bPXs0jw= X-Received: by 2002:a67:f70a:: with SMTP id m10mr281126vso.40.1634747574659; Wed, 20 Oct 2021 09:32:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyb1SuqyKsH6chzTYVAk7eamdBSFHL217pp06LR5d3w8lSvAjl4jHOWVdwLcKIxp872YRPgBJkYGO2kHMVQoVA= X-Received: by 2002:a67:f70a:: with SMTP id m10mr280863vso.40.1634747573057; Wed, 20 Oct 2021 09:32:53 -0700 (PDT) MIME-Version: 1.0 References: <20211018014059.13E65746353@zero.eik.bme.hu> <549ece11-990f-a19b-5dfe-508e315a6163@amsat.org> <31027ddc-b618-9628-d725-1516f7bfd098@amsat.org> <20211020143626.dvthmwizsljuwqz4@habkost.net> <20211020151824.ecl3etam425diyli@habkost.net> In-Reply-To: <20211020151824.ecl3etam425diyli@habkost.net> From: John Snow Date: Wed, 20 Oct 2021 12:32:42 -0400 Message-ID: Subject: Re: [PATCH] via-ide: Avoid expensive operations in irq handler To: Eduardo Habkost Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="00000000000063d51605cecb541c" Received-SPF: pass client-ip=170.10.133.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Huacai Chen , qemu-devel , Markus Armbruster , Gerd Hoffmann , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000063d51605cecb541c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Oct 20, 2021 at 11:18 AM Eduardo Habkost wrote: > On Wed, Oct 20, 2021 at 04:58:58PM +0200, BALATON Zoltan wrote: > > On Wed, 20 Oct 2021, Eduardo Habkost wrote: > > > On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daud=C3=A9= wrote: > > > > On 10/18/21 11:51, BALATON Zoltan wrote: > > > > > On Mon, 18 Oct 2021, Philippe Mathieu-Daud=C3=A9 wrote: > > > > > > On 10/18/21 03:36, BALATON Zoltan wrote: > > > > > > > Cache the pointer to PCI function 0 (ISA bridge, that this ID= E > device > > > > > > > has to use for IRQs) in the PCIIDEState and pass that as the > opaque > > > > > > > data for the interrupt handler to eliminate both the need to > look up > > > > > > > function 0 at every interrupt and also a QOM type cast of the > opaque > > > > > > > pointer as that's also expensive (mainly due to qom-debug bei= ng > > > > > > > enabled by default). > > > > > > > > > > > > > > Suggested-by: Philippe Mathieu-Daud=C3=A9 > > > > > > > Signed-off-by: BALATON Zoltan > > > > > > > --- > > > > > > > hw/ide/via.c | 11 ++++++----- > > > > > > > include/hw/ide/pci.h | 1 + > > > > > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/ide/via.c b/hw/ide/via.c > > > > > > > index 82def819c4..30566bc409 100644 > > > > > > > --- a/hw/ide/via.c > > > > > > > +++ b/hw/ide/via.c > > > > > > > @@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState > *d) > > > > > > > > > > > > > > static void via_ide_set_irq(void *opaque, int n, int level) > > > > > > > { > > > > > > > - PCIDevice *d =3D PCI_DEVICE(opaque); > > > > > > > + PCIIDEState *d =3D opaque; > > > > > > > > > > > > > > if (level) { > > > > > > > - d->config[0x70 + n * 8] |=3D 0x80; > > > > > > > + d->parent_obj.config[0x70 + n * 8] |=3D 0x80; > > > > > > > } else { > > > > > > > - d->config[0x70 + n * 8] &=3D ~0x80; > > > > > > > + d->parent_obj.config[0x70 + n * 8] &=3D ~0x80; > > > > > > > } > > > > > > > > > > > > Better not to access parent_obj, so I'd rather keep the previou= s > > > > > > code as it. The rest is OK, thanks. If you don't want to respin > > > > > > I can fix and take via mips-next. > > > > > > > > > > Why not? If it's OK to access other fields why not parent_obj? Th= at > > > > > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d) after thi= s > > > > > patch. We know it's a PCIIDEState and has PCIDevice parent_obj > field > > > > > because we set the opaque pointer when adding this callback so I > think > > > > > this works and is the less expensive way. But feel free to change > it any > > > > > way you like and use it that way. I'd keep it as it is. > > > > > > > > My understanding of QOM is we shouldn't access internal states that > > > > way, because 1/ this makes object refactors harder and 2/ this is > > > > not the style/example we want in the codebase, but it doesn't seem > > > > documented, so Cc'ing Markus/Eduardo for confirmation. > > > > > > `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_obj is > > > just a QOM implementation detail). If there are real performance > > > reasons to avoid it, we need numbers to support that. > > > > OK I can try to do some measurement or go back to PCI_DEVICE() then. > > > > > Also, note that `OBJECT_CHECK(obj)` is just `return obj` if > > > CONFIG_QOM_CAST_DEBUG is disabled. > > > > But configure enables it by default even without --enable-debug so I > think > > most people don't even know and it's enabled practically everywhere > > (probably even in distros). Why can't we have it disabled by default if > it's > > a developer debug option and enable it only for developers where it's > useful > > to catch bugs? > > It's a trade-off, I don't think there's a right answer for > everybody. Personally, I'd say the benefits outweigh the risks > of disabling it for most users (especially the ones building QEMU > directly from source). > > I don't mean to say it's OK for CONFIG_QOM_CAST_DEBUG=3Dy builds to > have visibly poor performance. If the typecast above causes a > measurable performance impact, it might be reasonable to have a > workaround like: > > static void via_ide_set_irq(void *opaque, int n, int level) > { > PCIIDEState *ide =3D opaque; > PCIDevice *pci =3D opaque; > ... > } > > -- > Eduardo > > For the record here, I'm going to defer to consensus judgment between Eduardo, Philippe, and BALATON. Let me know when you're all happy with the patch. --js --00000000000063d51605cecb541c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Oct 20, 2021 at 11:18 AM Edua= rdo Habkost <ehabkost@redhat.com<= /a>> wrote:
O= n Wed, Oct 20, 2021 at 04:58:58PM +0200, BALATON Zoltan wrote:
> On Wed, 20 Oct 2021, Eduardo Habkost wrote:
> > On Mon, Oct 18, 2021 at 12:10:04PM +0200, Philippe Mathieu-Daud= =C3=A9 wrote:
> > > On 10/18/21 11:51, BALATON Zoltan wrote:
> > > > On Mon, 18 Oct 2021, Philippe Mathieu-Daud=C3=A9 wrote:=
> > > > > On 10/18/21 03:36, BALATON Zoltan wrote:
> > > > > > Cache the pointer to PCI function 0 (ISA brid= ge, that this IDE device
> > > > > > has to use for IRQs) in the PCIIDEState and p= ass that as the opaque
> > > > > > data for the interrupt handler to eliminate b= oth the need to look up
> > > > > > function 0 at every interrupt and also a QOM = type cast of the opaque
> > > > > > pointer as that's also expensive (mainly = due to qom-debug being
> > > > > > enabled by default).
> > > > > >
> > > > > > Suggested-by: Philippe Mathieu-Daud=C3=A9 <= ;f4bug@amsat.org&g= t;
> > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > > ---
> > > > > > =C2=A0hw/ide/via.c=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 | 11 ++++++-----
> > > > > > =C2=A0include/hw/ide/pci.h |=C2=A0 1 +
> > > > > > =C2=A02 files changed, 7 insertions(+), 5 del= etions(-)
> > > > > >
> > > > > > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > > > > > index 82def819c4..30566bc409 100644
> > > > > > --- a/hw/ide/via.c
> > > > > > +++ b/hw/ide/via.c
> > > > > > @@ -104,15 +104,15 @@ static void bmdma_setup= _bar(PCIIDEState *d)
> > > > > >
> > > > > > =C2=A0static void via_ide_set_irq(void *opaqu= e, int n, int level)
> > > > > > =C2=A0{
> > > > > > -=C2=A0=C2=A0=C2=A0 PCIDevice *d =3D PCI_DEVI= CE(opaque);
> > > > > > +=C2=A0=C2=A0=C2=A0 PCIIDEState *d =3D opaque= ;
> > > > > >
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 if (level) {
> > > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 d= ->config[0x70 + n * 8] |=3D 0x80;
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 d= ->parent_obj.config[0x70 + n * 8] |=3D 0x80;
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 } else {
> > > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 d= ->config[0x70 + n * 8] &=3D ~0x80;
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 d= ->parent_obj.config[0x70 + n * 8] &=3D ~0x80;
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 }
> > > > >
> > > > > Better not to access parent_obj, so I'd rather= keep the previous
> > > > > code as it. The rest is OK, thanks. If you don'= ;t want to respin
> > > > > I can fix and take via mips-next.
> > > >
> > > > Why not? If it's OK to access other fields why not = parent_obj? That
> > > > avoids the QOM cast PCI_DEVICE(opaque) or PCI_DEVICE(d)= after this
> > > > patch. We know it's a PCIIDEState and has PCIDevice= parent_obj field
> > > > because we set the opaque pointer when adding this call= back so I think
> > > > this works and is the less expensive way. But feel free= to change it any
> > > > way you like and use it that way. I'd keep it as it= is.
> > >
> > > My understanding of QOM is we shouldn't access internal = states that
> > > way, because 1/ this makes object refactors harder and 2/ th= is is
> > > not the style/example we want in the codebase, but it doesn&= #39;t seem
> > > documented, so Cc'ing Markus/Eduardo for confirmation. > >
> > `PCI_DEVICE(d)` is preferred instead `of d.parent_obj` (parent_ob= j is
> > just a QOM implementation detail).=C2=A0 If there are real perfor= mance
> > reasons to avoid it, we need numbers to support that.
>
> OK I can try to do some measurement or go back to PCI_DEVICE() then. >
> > Also, note that `OBJECT_CHECK(obj)` is just `return obj` if
> > CONFIG_QOM_CAST_DEBUG is disabled.
>
> But configure enables it by default even without --enable-debug so I t= hink
> most people don't even know and it's enabled practically every= where
> (probably even in distros). Why can't we have it disabled by defau= lt if it's
> a developer debug option and enable it only for developers where it= 9;s useful
> to catch bugs?

It's a trade-off, I don't think there's a right answer for
everybody.=C2=A0 Personally, I'd say the benefits outweigh the risks of disabling it for most users (especially the ones building QEMU
directly from source).

I don't mean to say it's OK for CONFIG_QOM_CAST_DEBUG=3Dy builds to=
have visibly poor performance.=C2=A0 If the typecast above causes a
measurable performance impact, it might be reasonable to have a
workaround like:

=C2=A0 static void via_ide_set_irq(void *opaque, int n, int level)
=C2=A0 {
=C2=A0 =C2=A0 =C2=A0 PCIIDEState *ide =3D opaque;
=C2=A0 =C2=A0 =C2=A0 PCIDevice *pci =3D opaque;
=C2=A0 =C2=A0 =C2=A0 ...
=C2=A0 }

--
Eduardo


For the record here, I'm going to = defer to consensus judgment between Eduardo, Philippe, and BALATON. Let me = know when you're all happy with the patch.

--j= s
--00000000000063d51605cecb541c--