Re: [PATCH RFC 2/8] public/io/netif: add directory for backend parameters

[Joao Martins <joao.m.martins@oracle.com>]

On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Joao Martins [mailto:joao.m.martins@oracle.com]
> > Sent: 02 November 2017 18:06
> > To: Xen Development List <xen-devel@lists.xen.org>
> > Cc: Joao Martins <joao.m.martins@oracle.com>; Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > <wei.liu2@citrix.com>
> > Subject: [PATCH RFC 2/8] public/io/netif: add directory for backend
> > parameters
> > 
> > The proposed directory provides a mechanism for tools to control the
> > maximum feature set of the device being provisioned by backend.
> > The parameters/features include offloading features, number of
> > queues etc.
> > 
> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > ---
> >  xen/include/public/io/netif.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> > index 2454448baa..a412e4771d 100644
> > --- a/xen/include/public/io/netif.h
> > +++ b/xen/include/public/io/netif.h
> > @@ -161,6 +161,22 @@
> >   */
> > 
> >  /*
> > + * The directory "require" maybe be created in backend path by tools
> > + * domain to override the maximum feature set that backend provides to
> > the
> > + * frontend. The children entries within this directory are features names
> > + * and the correspondent values that should be used backend as defaults
> > e.g.:
> > + *
> > + * /local/domain/X/backend/<domid>/<handle>/require
> > + * /local/domain/X/backend/<domid>/<handle>/require/multi-queue-
> > max-queues = "2"
> > + * /local/domain/X/backend/<domid>/<handle>/require/feature-no-csum-
> > offload = "1"
> > + *
> > + * In the example above, network backend will negotiate up to a maximum
> > of
> > + * two queues with frontend plus disabling IPv4 checksum offloading.
> > + *
> > + * This directory and its children entries shall only be visible to the backend.
> > + */
> > +
> 
> What should happen if the toolstack sets something in 'require' that
> the backend cannot provide? I don't see anything in your RFC patches
> to check that the backend has responded appropriately to the keys.

Hmm, you're right that this RFC doesn't handle that properly - but for the
ones the backend provide I had suggested (albeit not implemented here)
back in the other thread that we could compare the values of feature in
"require" with the one announced to the frontend. But well this wouldn't
cover the non-provided ones, and possibly would fall a bit as a hack.

I could change the format of the entries within "require"
directory to be e.g. "<id>-<feature-name> = <feature-value>" and the
acknowledgement entry would come in the form "<id>-status
= <error code>". Consequently the lack of a "<id>-status" entry would
have a stronger semantic i.e. unsupported and ignored. The toolstack then would have
means to check whether the feature was really succesfull set as desired
or not. But then one question comes to mind: should the backend be
prevented to init in the event that the features requested fail to be
set? In which case uevent (on Linux) isn't triggered and xenbus state doesn't
get changed and toolstack would fail with timeout later on.

Also, a nice thing of this stuff is that we could also use this to set
set backend implementation specific parameters that are not
described or relevant in I/O specs. But then I start to wonder where would
be the correct place for backends to specify its maximum feature set of
changeable entries? Maybe:

/local/domain/X/backend/vif/features/<feature-name>
/local/domain/X/backend/vif/features/<feature-name>-desc = "Description
of <feature-name>"

Cheers,
Joao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel