On Thu, 26 Jul 2018 17:30:07 +0200
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> On Thu, 26 Jul 2018 15:50:28 +0200
> Erik Skultety <eskultet@xxxxxxxxxx> wrote:
>
> > On Tue, Jul 24, 2018 at 11:44:40AM -0600, Alex Williamson wrote:
> > > On Fri, 20 Jul 2018 10:19:24 +0800
> > > Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx> wrote:
> > >
> > > > Current mdev device create interface depends on fixed mdev type, which get uuid
> > > > from user to create instance of mdev device. If user wants to use customized
> > > > number of resource for mdev device, then only can create new mdev type for that
> > > > which may not be flexible. This requirement comes not only from to be able to
> > > > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > > > virtualization which would use vfio/mdev to be able to allocate arbitrary
> > > > resources on mdev instance. More info on [1] [2] [3].
> > > >
> > > > To allow to create user defined resources for mdev, it trys to extend mdev
> > > > create interface by adding new "instances=xxx" parameter following uuid, for
> > > > target mdev type if aggregation is supported, it can create new mdev device
> > > > which contains resources combined by number of instances, e.g
> > > >
> > > > echo "<uuid>,instances=10" > create
> > > >
> > > > VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> > > > can support this setting. If no "aggregation" attribute found for mdev type,
> > > > previous behavior is still kept for one instance allocation. And new sysfs
> > > > attribute "instances" is created for each mdev device to show allocated number.
> > > >
> > > > This trys to create new KVMGT type with minimal vGPU resources which can be
> > > > combined with "instances=x" setting to allocate for user wanted resources.
> > >
> > > "instances" makes me think this is arg helps to create multiple mdev
> > > instances rather than consuming multiple instances for a single mdev.
> > > You're already exposing the "aggregation" attribute, so doesn't
> > > "aggregate" perhaps make more sense as the create option? We're asking
> > > the driver to aggregate $NUM instances into a single mdev. The mdev
> > > attribute should then perhaps also be "aggregated_instances".
>
> I agree, that seems like a better naming scheme.
>
> (...)
>
> > > I'm curious what libvirt folks and Kirti think of this, it looks like
> > > it has a nice degree of backwards compatibility, both in the sysfs
> > > interface and the vendor driver interface. Thanks,
> >
> > Since libvirt doesn't have an API to create mdevs yet, this doesn't pose an
> > issue for us at the moment. I see this adds new optional sysfs attributes which
> > we could expose within our device capabilities XML, provided it doesn't use a
> > free form text, like the description attribute does.
>
> One thing I noticed is that we have seem to have an optional (?)
> vendor-driver created "aggregation" attribute (which always prints
> "true" in the Intel driver). Would it be better or worse for libvirt if
> it contained some kind of upper boundary or so?
Ultimately the aggregation value should be fully specified in
Documentation/ABI, but doesn't the kernel generally use 'Y' or 'N' for
boolean attributes in sysfs? Maybe mdev core can handle the attribute
since it should exist any time the create_with_instances callback is
provided.
> Additionally, would it
> be easier if the "create" attribute always accepted
> ",instances=1" (calling the .create ops in that case?)
Unless I misunderstand the code or the question, I think this is
exactly what happens:
+ if (instances > 1)
+ ret = parent->ops->create_with_instances(kobj, mdev, instances);
+ else
+ ret = parent->ops->create(kobj, mdev);
And elsewhere:
+ if (instances > 1 && !parent->ops->create_with_instances) {
+ ret = -EINVAL;
+ goto mdev_fail;
+ }
If the mdev core supplied the aggregation attribute, then the presence
of the attribute could indicate to userspace whether it can always
provide an instance (aggregate) count, even if limited to '1' when 'N',
for that mdev type. Thanks,
Alex
