Re: [PATCH 2/4] sysctl/libxl: Add interface for returning IO topology data

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/12/14 16:26, Boris Ostrovsky wrote:
> On 12/04/2014 06:55 AM, Dario Faggioli wrote:
>> On Tue, 2014-12-02 at 16:34 -0500, Boris Ostrovsky wrote:
>>> Make XEN_SYSCTL_topologyinfo more generic so that it can return both
>>> CPU and IO topology (support for returning the latter is added in the
>>> subsequent patch)
>>>
>> Is it always the case that we need the full information (i.e., both CPU
>> and IO topology), at all levels above Xen?
>>
>> I appreciate the performance implications (one hcall instead of two) in
>> cases where we do, but I still think, as Andrew suggested, that having a
>> new XEN_SYSCTL_iotopology (or something like that) and leaving
>> *_topologyinfo alone would make a better low-level interface.
>>
>> All IMHO, of course.
>
> I don't feel strongly either way so I can go that route (and it will
> make patch 4 completely unnecessary).
>
> (I am not sure though I understood Andrew's reasoning for splitting
> into two sysctls.

The two bits of information are different, and it is perfectly
reasonable to want the cpu information at one point, but the io
information at another.

As it stands, you appear to mandate that the caller wants the cpu
information, which is not true.

>From a separate point of view, while the sysctl abi version does allow
us to make changes like this, it makes a mess for valgrind and strace to
have radically different hypercalls with the same name, separated by
only the interface version. 

>
>>> To do so move (and rename) previously used cpu_to_core/cpu_to_socket/
>>> cpu_to_node into struct xen_sysctl_cputopo and move it, together with
>>> newly added struct xen_sysctl_iotopo, to xen_sysctl_topologyinfo.
>>>
>>> Add libxl_get_topology() to handle new interface and modify
>>> libxl_get_cpu_topology() to be a wrapper around it.
>>>
>> This is, I think, useful, and I'd go for it, even if we adding a new
>> hypercall at the Xen and xc level.
>>
>> Of course, it would work the other way round: the new libxl function
>> would wrap the existing libxl_get_cpu_topology() and a new libxl call
>> (something like libxl_get_io_topology() ?).
>>
>>> Adjust xenpm and python's low-level C libraries for new interface.
>>>
>> All in one patch? Wouldn't splitting it at least in two (hypervisor and
>> tools parts) be better? If it were me, I'd probably split even more...
>
> I could not split it because this patch changes sysctl interface (more
> specifically, xen_sysctl_topologyinfo_t/xc_topologyinfo_t) so anyone
> who uses this struct needed to be updated at the same time.
>
> Of course, if I were to leave current interface intact and add another
> sysctl for IO topology then this will not be necessary

This is the other reason why change simply for changes sake in the
interface is best avoided.  The unstable API goes very deep into the
toolstack.

>
>>
>>> This change requires bumping XEN_SYSCTL_INTERFACE_VERSION
>>>
>> Which would not be the case if we take the approach of adding a new,
>> iotopology specific, hcall, would it?
>
> I would think that any changes to a public interface, including adding
> a new function, require new version.
>
> (And if we get a new version, even if we split topology into CPU and
> IO sysctls, I'd still like to put cpu_to_[core|socket||node] into a
> single structure).

This would certainly decrease the faff both the toostack and hypervisor
have to go to when passing the parameters, and I think it would be a
good improvement.

~Andrew