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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

(Three months after you sent this, sorry...)

On Mon, Nov 06, 2017 at 12:33:06PM +0000, Joao Martins wrote:
> On Mon, Nov 06, 2017 at 10:33:59AM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > > Sent: 02 November 2017 18:06
> > > To: Xen Development List <xen-devel@xxxxxxxxxxxxx>
> > > Cc: Joao Martins <joao.m.martins@xxxxxxxxxx>; Konrad Rzeszutek Wilk
> > > <konrad.wilk@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu
> > > <wei.liu2@xxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > ---
> > >  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.
> 

I think the backend should not proceed if it can't meet the
requirements. But to be clear I also don't think the timeout behaviour
should be used to determine if the setting is successful because it is
asking other part of the system to pick up the slack and system
administrators would be left in the dark (unless there is easily
accessible message that can be obtained by libxl to return to system
administrators).

> 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>"

No idea. I'm very bad at naming things.

Wei.

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