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

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 02/06/2018 05:12 PM, Wei Liu wrote:
> (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).

That timeout behaviour is already there *I think* (or maybe I have the wrong
impression)? The alternative is to trigger the uevent and add more logic on the
hotplug script to check if the parameters were set according to config, but OTOH
you add more complexity there. Or perhaps we can check that the backend set to
its state to Unknown (or some other state) and that determines the failure - but
still no uevent should be triggered. Unless you had something else in your mind?

        Joao

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