Re: [PATCH v3 4/7] sysctl: Add sysctl interface for querying PCI topology

[Boris Ostrovsky <boris.ostrovsky%oracle.com@localhost>]

On 02/10/2015 09:54 AM, Andrew Cooper wrote:
> On 10/02/15 14:45, Boris Ostrovsky wrote:
> > On 02/10/2015 06:13 AM, Andrew Cooper wrote:
> > > On 09/02/15 20:04, Boris Ostrovsky wrote:
> > > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky%oracle.com@localhost>
> > > > ---
> > > >    xen/common/sysctl.c         |   73
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > >    xen/include/public/sysctl.h |   29 +++++++++++++++++
> > > >    2 files changed, 102 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> > > > index 30c5806..ea6557f 100644
> > > > --- a/xen/common/sysctl.c
> > > > +++ b/xen/common/sysctl.c
> > > > @@ -384,7 +384,80 @@ long
> > > > do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > > >            xfree(cputopo);
> > > >        }
> > > >        break;
> > > > +#ifdef HAS_PCI
> > > > +    case XEN_SYSCTL_pcitopoinfo:
> > > > +    {
> > > > +        xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
> > > > +        physdev_pci_device_t *devs;
> > > > +        uint8_t *nodes;
> > > > +        unsigned int first_dev, i;
> > > > +        int num_devs;
> > > > +
> > > > +        num_devs = ti->num_devs - ti->first_dev;
> > > > +
> > > > +        if ( guest_handle_is_null(ti->devs) ||
> > > > +             guest_handle_is_null(ti->nodes) ||
> > > > +             (num_devs <= 0) )
> > > > +        {
> > > > +            ret = -EINVAL;
> > > > +            break;
> > > > +        }
> > > > +
> > > > +        devs = xmalloc_array(physdev_pci_device_t, num_devs);
> > > > +        nodes = xmalloc_array(uint8_t, num_devs);
> > > You can do all of this without any memory allocation at all, which will
> > > simplify your error handling substantially.
> > >
> > > Something along the lines of
> > >
> > > for(...)
> > > {
> > >       copy one physdev_pci_device_t from the guest
> > >
> > >       do the lookup
> > >
> > >       copy one node id back to the guest
> > > }
> > I am trying to avoid doing multiple copies. For lots of devices (IIRC,
> > you said you had a system with a few thousand), having two copies per
> > loop will add up, I think.
> copy_to/from_guest() is not expensive.  It is a straight memcpy with an
> extable guards for pagefaults.

True, but still why do this inside a loop? xmalloc() of less than a page 
is not that expensive, is it?

(The downside is that when we have really lots of devices we may be 
asking for more than one page. I know that we try not to do this but 
again, I think the expense would be amortised over long loops.).

-boris