[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v1 05/11] bandwidth: Create network (un)plug functions



On 30.11.2012 19:43, Laine Stump wrote:
> On 11/19/2012 11:51 AM, Michal Privoznik wrote:
>> Network should be notified if we plug in or unplug an
>> interface, so it can perform some action, e.g. set/unset
>> network part of QoS.
>> ---
>>  src/Makefile.am             |    7 ++-
>>  src/conf/domain_conf.h      |    1 +
>>  src/conf/network_conf.c     |    1 +
>>  src/conf/network_conf.h     |    3 +
>>  src/libvirt_network.syms    |    8 ++
>>  src/network/bridge_driver.c |  165 +++++++++++++++++++++++++++++++++++++++++++
>>  src/network/bridge_driver.h |   10 ++-
>>  src/qemu/qemu_command.c     |   32 ++++++---
>>  src/qemu/qemu_process.c     |   12 +++
>>  9 files changed, 225 insertions(+), 14 deletions(-)
>>  create mode 100644 src/libvirt_network.syms
>>

>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>> index 3e46304..8a8d1e4 100644
>> --- a/src/conf/network_conf.h
>> +++ b/src/conf/network_conf.h
>> @@ -221,6 +221,9 @@ struct _virNetworkObj {
>>  
>>      virNetworkDefPtr def; /* The current definition */
>>      virNetworkDefPtr newDef; /* New definition to activate at shutdown */
>> +
>> +    unsigned int class_id; /* next class ID for QoS */
> 
> Okay, so this is just a monotonically increasing counter so that each
> interface gets a new and different class_id. Maybe you should call it
> "nextFreeClassID" or something like that, so that everyone understands
> it's not a class id used by the network. Or you might want to do this
> with a bitmap so you can re-use id's of interfaces that are
> disconnected. (can virBitmaps being dynamically expanded?)
> 
>> +    unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
> 
> And you keep track of the sum of all floors here.
> 
> So, what happens if libvirtd is restarted? It looks like they both go
> back to their initial values.

We think alike :) In the next patches I am fixing this. class_id becomes
a virBitmap and among with floor_sum gets stored into network status
XML. I've split it into several patches because changes are big (in
number of lines at least) and I think this way it's easier to review.

> 
> And what about hotplug?
> 
> This is a similar problem to the pool of interfaces used by
> macvtap/hostdev modes - a network with an interface pool needs to have
> the "inuse" counters of each interface refreshed whenever libvirtd
> restarts. So the necessary hooks are already in place:
> 
>    networkAllocateActualDevice - called any time an interface with
> type='network' is initially
>        allocated and connected.
> 
>    networkNotifyActualDevice - called for each type='network' interface
> of each active domain
>        any time libvirtd restarts
> 
>    networkReleaseActualDevice - called for every type='network'
> interface as it is being
>        disconnected and destroyed.
> 
> These are called for *all* type='network' interfaces, not just those
> that need to allocate a physical netdev from a pool.
> 
> Rather than adding your own new hooks (and figuring out all the places
> they need to be called), I think it would be better to use the existing
> hooks (perhaps calling a reduced/renamed version of your functions,
> which can possibly be moved over to ). That will have two advantages:
> 
> 1) It will assure that the bandwidth functions are called at all the
> right times, including hotplug/unplug, and libvirtd restart
> 
> 2) It will continue the process of consolidating all network-related
> functionality need for these three events into 3 functions which may
> some day be moved into their own daemom with a public (within libvirt
> anyway) API accessible via RPC (thus allowing non-privileged libvirts to
> have full networking functionality).
> 

Okay, renamed to networkPlugBandwidth and networkUnplugBandwidth and
inserted into networkAllocateActualDevice and networkReleaseActualDevice
respectively.


> Note that you will probably want to save the interface class_id in the
> iface->actual (as a matter of fact, in
> iface->data.network.actual->bandwidth, which can be retrieved with
> virDomainNetGetActualBandwidth()) so that it's saved properly in the
> interface's state without polluting the interface's config. Then it will
> be read back from the state file during the restart and available during
> networkNotifyActualDevice() of each interface. I guess you can then
> re-set the network->class_id by checking interface->class_id and setting
> 
>    network->class_id = MAX(network->class_id, iface->class_id + 1);
> 
> for each encountered interface (or add it to a bitmap, if a) bitmaps can
> be enlarged dynamically or b) we can determine a reasonable maximum
> limit on number of active domains). At the same time, you'll recompute
> the network->floor_sum iteratively as the network is called with a
> notify for each interface.
> 

>> ...

> 
> To summarize: I think this needs to be refactored to be integrated into
> the network*ActualDevice() functions so that the bookkeeping is properly
> handled in all cases, including hotplug and libvirtd restart.
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]