Re: [Xen-devel] [PATCH 4/5] print: introduce a format specifier for pci_sbdf_t

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 24, 2019 at 04:36:42AM -0600, Jan Beulich wrote:
> >>> On 10.05.19 at 18:10, <roger.pau@xxxxxxxxxx> wrote:
> > The new format specifier is '%pp', and prints a pci_sbdf_t using the
> > seg:bus:dev.func format. Replace all SBDFs printed using
> > '%04x:%02x:%02x.%u' to use the new format specifier.
> 
> So on the positive side Linux doesn't use 'p' yet, so we're only at risk
> of a future conflict. However, having to pass a 64-bit pointer just
> to print a 32-bit entity seems rather wasteful to me.

I think there are two issues here, one that you mention is the waste
of using a 64bit pointer to pass a 32bit value, the other one would
be the unneeded pointer indirection.

I've thought about the same, but then realized that this is used
(always?) to output messages, which is in itself a slow operation, and
the pointer indirection is likely negligible compared to the cost of
the rest of the operation. The usage of 64bit is also wasteful, but
again this shouldn't be a hotpath anyway, and this code replaces the
usage of four parameters to print a SBDF into a single one.

> Since we can't
> use entirely new format specifiers, did you consider (ab)using one
> we rarely use, like %o, suffixed similarly like we do for %p? The
> extension could be restricted to apply only when neither field width
> nor precision nor any flags were specified, i.e. only to plain %o (at
> least initially).
> 
> We'd then have something along the lines of
> 
> #define PRI_sbdf "op"
> #define PRI_SBDF(v) ((v).sbdf)
> 
> and
> 
>     printk("%" PRI_sbdf ": ...\n", PRI_SBDF(pdev->sbdf), ...);

I have to admit this looks more hacky than my current suggestion IMO.
The %p formatter overloading seems more standard and expected rather
than overloading %o.

Plus, one thing I didn't realize, I think Xen could even use %pci to
print and SBDF, which will make it even clearer.

> > --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> > @@ -717,9 +717,8 @@ static u16 __init parse_ivhd_device_special(
> >          return 0;
> >      }
> >  
> > -    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle 
> > %#x\n",
> > -                    seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> > -                    special->variety, special->handle);
> > +    AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n",
> > +                    &PCI_SBDF2_T(seg, bdf), special->variety, 
> > special->handle);
> 
> The inefficiency of the%p-based approach is perhaps best seen with an
> example like this: The compiler will have to instantiate an unnamed variable
> on the stack to hold the value of the compound literal, just to be able to
> take its address.

Right, and such iommu debug is enabled or disabled at runtime, so
regardless of whether iommu debug is enabled or not you will end up
with such stack variable pointer.

In this specific example such usage is not that bad because that's a
boot time only message, but I'm sure there are others which are not
boot time only. The vast majority of messages however don't require
the usage of a literal.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel