In the past there have been spotted some shortcomings in the Xenstore
interface, which should be repaired. Those are in detail:

- Using driver domains for large number of domains needs per domain
  Xenstore quota [1]. The feedback sent was rather slim (one reply),
  but it was preferring a new set of wire commands.

- XSA-349 [2] has shown that the current definition of watches is not
  optimal, as it will trigger lots of events when a single one would
  suffice: for detecting new backend devices the backends in the Linux
  kernel are registering a watch for e.g. "/local/domain/0/backend"
  which will fire for ANY sub-node written below this node (on a test
  machine this added up to 91 watch events for only 3 devices).
  This can be limited dramatically by extending the XS_WATCH command
  to take another optional parameter specifying the depth of
  subdirectories to be considered for sending watch events ("0" would
  trigger a watch event only if the watched node itself being written).

- New features like above being added might make migration of guests
  between hosts with different Xenstore variants harder, so it should
  be possible to set the available feature set per domain. For socket
  connections it should be possible to read the available features.

- The special watches @introduceDomain and @releaseDomain are rather
  cumbersome to use, as they only tell you that SOME domain has been
  introduced/released. Any consumer of those watches needs to scan
  all domains on the host in order to find out the domid, causing
  significant pressure on the dominfo hypercall (imagine a system
  with 1000 domains running and one domain dying - there will be more
  than 1000 watch events triggered and 1000 xl daemons will try to
  find out whether "their" domain has died). Those watches should be
  enhanced to optionally be specific to a single domain and to let the
  event carry the related domid.

As some of those extensions will need to be considered in the Xenstore
migration stream, they should be defined in one go (in fact the 4th one
wouldn't need that, but it can easily be connected to the 2nd one).
As such extensions need to be flagged in the "features" in the ring
page anyway, it is fine to implement them independently.

Add the documentation of the new commands/features.

[1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
[2]: http://xenbits.xen.org/xsa/advisory-349.html

Changes in V2:
- added new patch 1
- remove feature bits for dom0-only features
- get-features without domid returns Xenstore supported features
- get/set-quota without domid for global quota access

Juergen Gross (4):
  tools/xenstore: modify feature bit specification in xenstore-ring.txt
  tools/xenstore: add documentation for new set/get-feature commands
  tools/xenstore: add documentation for new set/get-quota commands
  tools/xenstore: add documentation for extended watch command

 docs/misc/xenstore-ring.txt | 10 ++++----
 docs/misc/xenstore.txt      | 47 ++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.35.3

Hi,

It seems that this series [1] has been stale for a while with actions needed from
the maintainers (review needed). So sending this email as a gentle reminder.
Thanks!

[1] https://patchwork.kernel.org/project/xen-devel/list/?series=645480

Kind regards,
Henry

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Juergen Gross
> Subject: [PATCH v2 0/4] tools/xenstore: add some new features to the
> documentation
> 
> In the past there have been spotted some shortcomings in the Xenstore
> interface, which should be repaired. Those are in detail:
> 
> - Using driver domains for large number of domains needs per domain
>   Xenstore quota [1]. The feedback sent was rather slim (one reply),
>   but it was preferring a new set of wire commands.
> 
> - XSA-349 [2] has shown that the current definition of watches is not
>   optimal, as it will trigger lots of events when a single one would
>   suffice: for detecting new backend devices the backends in the Linux
>   kernel are registering a watch for e.g. "/local/domain/0/backend"
>   which will fire for ANY sub-node written below this node (on a test
>   machine this added up to 91 watch events for only 3 devices).
>   This can be limited dramatically by extending the XS_WATCH command
>   to take another optional parameter specifying the depth of
>   subdirectories to be considered for sending watch events ("0" would
>   trigger a watch event only if the watched node itself being written).
> 
> - New features like above being added might make migration of guests
>   between hosts with different Xenstore variants harder, so it should
>   be possible to set the available feature set per domain. For socket
>   connections it should be possible to read the available features.
> 
> - The special watches @introduceDomain and @releaseDomain are rather
>   cumbersome to use, as they only tell you that SOME domain has been
>   introduced/released. Any consumer of those watches needs to scan
>   all domains on the host in order to find out the domid, causing
>   significant pressure on the dominfo hypercall (imagine a system
>   with 1000 domains running and one domain dying - there will be more
>   than 1000 watch events triggered and 1000 xl daemons will try to
>   find out whether "their" domain has died). Those watches should be
>   enhanced to optionally be specific to a single domain and to let the
>   event carry the related domid.
> 
> As some of those extensions will need to be considered in the Xenstore
> migration stream, they should be defined in one go (in fact the 4th one
> wouldn't need that, but it can easily be connected to the 2nd one).
> As such extensions need to be flagged in the "features" in the ring
> page anyway, it is fine to implement them independently.
> 
> Add the documentation of the new commands/features.
> 
> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
> [2]: http://xenbits.xen.org/xsa/advisory-349.html
> 
> Changes in V2:
> - added new patch 1
> - remove feature bits for dom0-only features
> - get-features without domid returns Xenstore supported features
> - get/set-quota without domid for global quota access
> 
> Juergen Gross (4):
>   tools/xenstore: modify feature bit specification in xenstore-ring.txt
>   tools/xenstore: add documentation for new set/get-feature commands
>   tools/xenstore: add documentation for new set/get-quota commands
>   tools/xenstore: add documentation for extended watch command
> 
>  docs/misc/xenstore-ring.txt | 10 ++++----
>  docs/misc/xenstore.txt      | 47 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> --
> 2.35.3
> 

On 27.05.2022 09:24, Juergen Gross wrote:
> In the past there have been spotted some shortcomings in the Xenstore
> interface, which should be repaired. Those are in detail:
> 
> - Using driver domains for large number of domains needs per domain
>   Xenstore quota [1]. The feedback sent was rather slim (one reply),
>   but it was preferring a new set of wire commands.
> 
> - XSA-349 [2] has shown that the current definition of watches is not
>   optimal, as it will trigger lots of events when a single one would
>   suffice: for detecting new backend devices the backends in the Linux
>   kernel are registering a watch for e.g. "/local/domain/0/backend"
>   which will fire for ANY sub-node written below this node (on a test
>   machine this added up to 91 watch events for only 3 devices).
>   This can be limited dramatically by extending the XS_WATCH command
>   to take another optional parameter specifying the depth of
>   subdirectories to be considered for sending watch events ("0" would
>   trigger a watch event only if the watched node itself being written).
> 
> - New features like above being added might make migration of guests
>   between hosts with different Xenstore variants harder, so it should
>   be possible to set the available feature set per domain. For socket
>   connections it should be possible to read the available features.
> 
> - The special watches @introduceDomain and @releaseDomain are rather
>   cumbersome to use, as they only tell you that SOME domain has been
>   introduced/released. Any consumer of those watches needs to scan
>   all domains on the host in order to find out the domid, causing
>   significant pressure on the dominfo hypercall (imagine a system
>   with 1000 domains running and one domain dying - there will be more
>   than 1000 watch events triggered and 1000 xl daemons will try to
>   find out whether "their" domain has died). Those watches should be
>   enhanced to optionally be specific to a single domain and to let the
>   event carry the related domid.
> 
> As some of those extensions will need to be considered in the Xenstore
> migration stream, they should be defined in one go (in fact the 4th one
> wouldn't need that, but it can easily be connected to the 2nd one).
> As such extensions need to be flagged in the "features" in the ring
> page anyway, it is fine to implement them independently.
> 
> Add the documentation of the new commands/features.
> 
> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
> [2]: http://xenbits.xen.org/xsa/advisory-349.html
> 
> Changes in V2:
> - added new patch 1
> - remove feature bits for dom0-only features
> - get-features without domid returns Xenstore supported features
> - get/set-quota without domid for global quota access
> 
> Juergen Gross (4):
>   tools/xenstore: modify feature bit specification in xenstore-ring.txt
>   tools/xenstore: add documentation for new set/get-feature commands
>   tools/xenstore: add documentation for new set/get-quota commands
>   tools/xenstore: add documentation for extended watch command

Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
seeing there had been R-b with no other follow-up (leaving aside the v2)
in a long time. Please advise if I should revert the commits. I'm sorry
for the confusion. (I also wonder why the R-b weren't carried over to v2.)

Jan

Hi Jan,

On 18/07/2022 17:12, Jan Beulich wrote:
> On 27.05.2022 09:24, Juergen Gross wrote:
>> In the past there have been spotted some shortcomings in the Xenstore
>> interface, which should be repaired. Those are in detail:
>>
>> - Using driver domains for large number of domains needs per domain
>>    Xenstore quota [1]. The feedback sent was rather slim (one reply),
>>    but it was preferring a new set of wire commands.
>>
>> - XSA-349 [2] has shown that the current definition of watches is not
>>    optimal, as it will trigger lots of events when a single one would
>>    suffice: for detecting new backend devices the backends in the Linux
>>    kernel are registering a watch for e.g. "/local/domain/0/backend"
>>    which will fire for ANY sub-node written below this node (on a test
>>    machine this added up to 91 watch events for only 3 devices).
>>    This can be limited dramatically by extending the XS_WATCH command
>>    to take another optional parameter specifying the depth of
>>    subdirectories to be considered for sending watch events ("0" would
>>    trigger a watch event only if the watched node itself being written).
>>
>> - New features like above being added might make migration of guests
>>    between hosts with different Xenstore variants harder, so it should
>>    be possible to set the available feature set per domain. For socket
>>    connections it should be possible to read the available features.
>>
>> - The special watches @introduceDomain and @releaseDomain are rather
>>    cumbersome to use, as they only tell you that SOME domain has been
>>    introduced/released. Any consumer of those watches needs to scan
>>    all domains on the host in order to find out the domid, causing
>>    significant pressure on the dominfo hypercall (imagine a system
>>    with 1000 domains running and one domain dying - there will be more
>>    than 1000 watch events triggered and 1000 xl daemons will try to
>>    find out whether "their" domain has died). Those watches should be
>>    enhanced to optionally be specific to a single domain and to let the
>>    event carry the related domid.
>>
>> As some of those extensions will need to be considered in the Xenstore
>> migration stream, they should be defined in one go (in fact the 4th one
>> wouldn't need that, but it can easily be connected to the 2nd one).
>> As such extensions need to be flagged in the "features" in the ring
>> page anyway, it is fine to implement them independently.
>>
>> Add the documentation of the new commands/features.
>>
>> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
>> [2]: http://xenbits.xen.org/xsa/advisory-349.html
>>
>> Changes in V2:
>> - added new patch 1
>> - remove feature bits for dom0-only features
>> - get-features without domid returns Xenstore supported features
>> - get/set-quota without domid for global quota access
>>
>> Juergen Gross (4):
>>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
>>    tools/xenstore: add documentation for new set/get-feature commands
>>    tools/xenstore: add documentation for new set/get-quota commands
>>    tools/xenstore: add documentation for extended watch command
> 
> Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
> seeing there had been R-b with no other follow-up (leaving aside the v2)
> in a long time. Please advise if I should revert the commits. I'm sorry
> for the confusion. (I also wonder why the R-b weren't carried over to v2.)

patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the 
overall interaction is different. So I don't think the reviewed-by 
should have been carried.

I had some concerns in v1 which were addressed in v2. I have reviewed v2 
a while ago. From my perspective, patch #1, #3, #4 are ready to go. 
Patch #2 needs a respin and we also need to clarify the integration with 
migration/live-update.

As you committed, I would be OK if this is addressed in a follow-up 
series. But this *must* be addressed by the time 4.17 is released 
because otherwise we will commit ourself to a broken interface. @Henry, 
please add this in the blocker list.

Cheers,

-- 
Julien Grall

On 18.07.2022 18:28, Julien Grall wrote:
> On 18/07/2022 17:12, Jan Beulich wrote:
>> On 27.05.2022 09:24, Juergen Gross wrote:
>>> In the past there have been spotted some shortcomings in the Xenstore
>>> interface, which should be repaired. Those are in detail:
>>>
>>> - Using driver domains for large number of domains needs per domain
>>>    Xenstore quota [1]. The feedback sent was rather slim (one reply),
>>>    but it was preferring a new set of wire commands.
>>>
>>> - XSA-349 [2] has shown that the current definition of watches is not
>>>    optimal, as it will trigger lots of events when a single one would
>>>    suffice: for detecting new backend devices the backends in the Linux
>>>    kernel are registering a watch for e.g. "/local/domain/0/backend"
>>>    which will fire for ANY sub-node written below this node (on a test
>>>    machine this added up to 91 watch events for only 3 devices).
>>>    This can be limited dramatically by extending the XS_WATCH command
>>>    to take another optional parameter specifying the depth of
>>>    subdirectories to be considered for sending watch events ("0" would
>>>    trigger a watch event only if the watched node itself being written).
>>>
>>> - New features like above being added might make migration of guests
>>>    between hosts with different Xenstore variants harder, so it should
>>>    be possible to set the available feature set per domain. For socket
>>>    connections it should be possible to read the available features.
>>>
>>> - The special watches @introduceDomain and @releaseDomain are rather
>>>    cumbersome to use, as they only tell you that SOME domain has been
>>>    introduced/released. Any consumer of those watches needs to scan
>>>    all domains on the host in order to find out the domid, causing
>>>    significant pressure on the dominfo hypercall (imagine a system
>>>    with 1000 domains running and one domain dying - there will be more
>>>    than 1000 watch events triggered and 1000 xl daemons will try to
>>>    find out whether "their" domain has died). Those watches should be
>>>    enhanced to optionally be specific to a single domain and to let the
>>>    event carry the related domid.
>>>
>>> As some of those extensions will need to be considered in the Xenstore
>>> migration stream, they should be defined in one go (in fact the 4th one
>>> wouldn't need that, but it can easily be connected to the 2nd one).
>>> As such extensions need to be flagged in the "features" in the ring
>>> page anyway, it is fine to implement them independently.
>>>
>>> Add the documentation of the new commands/features.
>>>
>>> [1]: https://lists.xen.org/archives/html/xen-devel/2020-06/msg00291.html
>>> [2]: http://xenbits.xen.org/xsa/advisory-349.html
>>>
>>> Changes in V2:
>>> - added new patch 1
>>> - remove feature bits for dom0-only features
>>> - get-features without domid returns Xenstore supported features
>>> - get/set-quota without domid for global quota access
>>>
>>> Juergen Gross (4):
>>>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
>>>    tools/xenstore: add documentation for new set/get-feature commands
>>>    tools/xenstore: add documentation for new set/get-quota commands
>>>    tools/xenstore: add documentation for extended watch command
>>
>> Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
>> seeing there had been R-b with no other follow-up (leaving aside the v2)
>> in a long time. Please advise if I should revert the commits. I'm sorry
>> for the confusion. (I also wonder why the R-b weren't carried over to v2.)
> 
> patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the 
> overall interaction is different. So I don't think the reviewed-by 
> should have been carried.
> 
> I had some concerns in v1 which were addressed in v2. I have reviewed v2 
> a while ago. From my perspective, patch #1, #3, #4 are ready to go. 
> Patch #2 needs a respin and we also need to clarify the integration with 
> migration/live-update.
> 
> As you committed, I would be OK if this is addressed in a follow-up 
> series. But this *must* be addressed by the time 4.17 is released 
> because otherwise we will commit ourself to a broken interface. @Henry, 
> please add this in the blocker list.

If you hadn't answered, I would have reverted these right away this
morning, in particular to remove the (now wrong) feature bit part
(patches 2 and 3 have dropped their feature bit additions in v2).
If you nevertheless think an incremental update is going to be okay,
I'll leave things alone. But personally I think this mistake of mine
would better be corrected immediately.

Jan

Hi Jan,

On 19/07/2022 06:58, Jan Beulich wrote:
> On 18.07.2022 18:28, Julien Grall wrote:
>> On 18/07/2022 17:12, Jan Beulich wrote:
>>> On 27.05.2022 09:24, Juergen Gross wrote:
>> As you committed, I would be OK if this is addressed in a follow-up
>> series. But this *must* be addressed by the time 4.17 is released
>> because otherwise we will commit ourself to a broken interface. @Henry,
>> please add this in the blocker list.
> 
> If you hadn't answered, I would have reverted these right away this
> morning, in particular to remove the (now wrong) feature bit part
> (patches 2 and 3 have dropped their feature bit additions in v2).
> If you nevertheless think an incremental update is going to be okay,
> I'll leave things alone. But personally I think this mistake of mine
> would better be corrected immediately.

I wasn't arguing against a revert and it looks like Juergen is away for 
the next 2 weeks. So if you prefer to correct the mistake now, then 
please revert it.

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 0/4] tools/xenstore: add some new features to the
> documentation
> 
> Hi Jan,
> 
> On 18/07/2022 17:12, Jan Beulich wrote:
> > On 27.05.2022 09:24, Juergen Gross wrote:
> >>
> >> Changes in V2:
> >> - added new patch 1
> >> - remove feature bits for dom0-only features
> >> - get-features without domid returns Xenstore supported features
> >> - get/set-quota without domid for global quota access
> >>
> >> Juergen Gross (4):
> >>    tools/xenstore: modify feature bit specification in xenstore-ring.txt
> >>    tools/xenstore: add documentation for new set/get-feature commands
> >>    tools/xenstore: add documentation for new set/get-quota commands
> >>    tools/xenstore: add documentation for extended watch command
> >
> > Hmm, looks like I did commit v1 of this series, not noticing the v2 _and_
> > seeing there had been R-b with no other follow-up (leaving aside the v2)
> > in a long time. Please advise if I should revert the commits. I'm sorry
> > for the confusion. (I also wonder why the R-b weren't carried over to v2.)
> 
> patch #1 is a new patch. The patch #2, #3, #4 have been reworded and the
> overall interaction is different. So I don't think the reviewed-by
> should have been carried.
> 
> I had some concerns in v1 which were addressed in v2. I have reviewed v2
> a while ago. From my perspective, patch #1, #3, #4 are ready to go.
> Patch #2 needs a respin and we also need to clarify the integration with
> migration/live-update.
> 
> As you committed, I would be OK if this is addressed in a follow-up
> series. But this *must* be addressed by the time 4.17 is released
> because otherwise we will commit ourself to a broken interface. @Henry,
> please add this in the blocker list.

Thank you very much for this information. I've added this in blocker list.
I will keep that in mind and send (proper) reminders during the timeline
of the 4.17 release process.

Kind regards,
Henry 

> 
> Cheers,
> 
> --
> Julien Grall