Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-08 Thread Alberto Garcia
On Wed 02 Aug 2017 12:57:04 PM CEST, Manos Pitsidianakis wrote:
>> At the moment I think throttle_groups_lock isn't strictly needed
>> because incref/decref callers hold the QEMU global mutex anyway.
>>
>> But code accessing throttle_groups still has to be disciplined.
>> Since throttle_groups_lock exists, please use it consistently in all
>> code paths.
>>
>> Alternatively you could remove the lock and document that
>> throttle_groups is protected by the global mutex.  What we can't do
>> is sometimes use throttle_groups_lock and sometimes not use it.
>
> If we use throttle_groups_lock in throttle_group_obj_init() then we
> must give it up in throttle_group_incref() and retake it in
> throttle_group_obj_init(). Maybe indeed it's better to drop
> throttle_groups_lock altogether, since the ThrottleGroup refcounting
> always happens in a QMP or startup/cleanup context.

I checked the code and I also don't see any manipulation of the group
list outside the global mutex, so you can remove throttle_groups_lock,
but please document very clearly that all these calls can only happen
when they are protected by the global mutex.

Berto



Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-03 Thread Manos Pitsidianakis

On Thu, Aug 03, 2017 at 01:17:01PM +0200, Kevin Wolf wrote:

Am 03.08.2017 um 12:53 hat Stefan Hajnoczi geschrieben:

On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > > throttle block filter drive easy creation and configuration of 
throttle
> > > > > > groups in QMP and cli.
> > > > > >
> > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > > struct for all throttle configuration needs in QMP.
> > > > > >
> > > > > > ThrottleGroups can be created via CLI as
> > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > where x-* are individual limit properties. Since we can't add 
non-scalar
> > > > > > properties in -object this interface must be used instead. However,
> > > > > > setting these properties must be disabled after initialization 
because
> > > > > > certain combinations of limits are forbidden and thus configuration
> > > > > > changes should be done in one transaction. The individual properties
> > > > > > will go away when support for non-scalar values in CLI is 
implemented
> > > > > > and thus are marked as experimental.
> > > > > >
> > > > > > ThrottleGroup also has a `limits` property that uses the 
ThrottleLimits
> > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > configuration in existing groups as follows:
> > > > > >
> > > > > > { "execute": "object-add",
> > > > > >   "arguments": {
> > > > > > "qom-type": "throttle-group",
> > > > > > "id": "foo",
> > > > > > "props" : {
> > > > > >   "limits": {
> > > > > >   "iops-total": 100
> > > > > >   }
> > > > > > }
> > > > > >   }
> > > > > > }
> > > > > > { "execute" : "qom-set",
> > > > > > "arguments" : {
> > > > > > "path" : "foo",
> > > > > > "property" : "limits",
> > > > > > "value" : {
> > > > > > "iops-total" : 99
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > This also means a group's configuration can be fetched with qom-get.
> > > > > >
> > > > > > ThrottleGroups can be anonymous which means they can't get accessed 
by
> > > > > > other users ie they will always be units instead of group (Because 
they
> > > > > > have one ThrottleGroupMember).
> > > > >
> > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > throttling.group= wasn't given.  So who will use anonymous 
single-drive
> > > > > mode?
> > > >
> > > > Manual filter nodes. It's possible to not pass a group name value and 
the
> > > > resulting group will be anonymous. Are you suggesting to move this 
change to
> > > > the throttle filter patch?
> > >
> > > What is the use case?  Human users will stick to the legacy syntax
> > > because it's convenient.  Management tools will use the filter
> > > explicitly in the future, and it's easy for them to choose a name.
> > >
> > > Unless there is a need for this case I'd prefer to make the group name
> > > mandatory.  That way there are less code paths to worry about.
> >
> > I think Kevin requested this though I don't really remember the use case.
>
> There is no legacy syntax for putting a throttle node anywhere but at
> the root of a BlockBackend. If you want to throttle e.g. only a specific
> backing file, you need to manually create a throttle node.
>
> (We tend to forget this occasionally, but the work you're doing is not
> only cleanup just for fun, but it's actually new features that enable
> new use cases by making everything more flexible.)

It's not clear to me from your reply whether you support anonymous
throttle groups or not.  It is possible to throttle arbitrary nodes in
the graph either way.


I think it would be nice for human users to have them, but on second
thought you're right that it's just syntactic sugar for an explicit
-object, so if you think there is any difficulty with supporting
anonymous groups, feel free to drop them.

Hm, actually... Does this mean that then the whole 'limits' option in
the throttle driver could go away, with all of the parsing code, and the
group name becomes mandatory? That certainly does look tempting.



Anonymous groups don't pose any difficulty. The only problem with groups 
not created with -object or object-add in general is that their limits 
cannot be set with qom-set afterwards; the throttle node will require a 
reopen or replacement with a new one. This is a good argument against 
doing throttle group manipulation in the driver. I don't know if that's 
very user friendly though.


signature.asc

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-03 Thread Kevin Wolf
Am 03.08.2017 um 12:53 hat Stefan Hajnoczi geschrieben:
> On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> > Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > > ThrottleGroup is converted to an object. This will allow the 
> > > > > > > future
> > > > > > > throttle block filter drive easy creation and configuration of 
> > > > > > > throttle
> > > > > > > groups in QMP and cli.
> > > > > > >
> > > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a 
> > > > > > > shared
> > > > > > > struct for all throttle configuration needs in QMP.
> > > > > > >
> > > > > > > ThrottleGroups can be created via CLI as
> > > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > > where x-* are individual limit properties. Since we can't add 
> > > > > > > non-scalar
> > > > > > > properties in -object this interface must be used instead. 
> > > > > > > However,
> > > > > > > setting these properties must be disabled after initialization 
> > > > > > > because
> > > > > > > certain combinations of limits are forbidden and thus 
> > > > > > > configuration
> > > > > > > changes should be done in one transaction. The individual 
> > > > > > > properties
> > > > > > > will go away when support for non-scalar values in CLI is 
> > > > > > > implemented
> > > > > > > and thus are marked as experimental.
> > > > > > >
> > > > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > > > ThrottleLimits
> > > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > > configuration in existing groups as follows:
> > > > > > >
> > > > > > > { "execute": "object-add",
> > > > > > >   "arguments": {
> > > > > > > "qom-type": "throttle-group",
> > > > > > > "id": "foo",
> > > > > > > "props" : {
> > > > > > >   "limits": {
> > > > > > >   "iops-total": 100
> > > > > > >   }
> > > > > > > }
> > > > > > >   }
> > > > > > > }
> > > > > > > { "execute" : "qom-set",
> > > > > > > "arguments" : {
> > > > > > > "path" : "foo",
> > > > > > > "property" : "limits",
> > > > > > > "value" : {
> > > > > > > "iops-total" : 99
> > > > > > > }
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > This also means a group's configuration can be fetched with 
> > > > > > > qom-get.
> > > > > > >
> > > > > > > ThrottleGroups can be anonymous which means they can't get 
> > > > > > > accessed by
> > > > > > > other users ie they will always be units instead of group 
> > > > > > > (Because they
> > > > > > > have one ThrottleGroupMember).
> > > > > >
> > > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > > throttling.group= wasn't given.  So who will use anonymous 
> > > > > > single-drive
> > > > > > mode?
> > > > > 
> > > > > Manual filter nodes. It's possible to not pass a group name value and 
> > > > > the
> > > > > resulting group will be anonymous. Are you suggesting to move this 
> > > > > change to
> > > > > the throttle filter patch?
> > > > 
> > > > What is the use case?  Human users will stick to the legacy syntax
> > > > because it's convenient.  Management tools will use the filter
> > > > explicitly in the future, and it's easy for them to choose a name.
> > > > 
> > > > Unless there is a need for this case I'd prefer to make the group name
> > > > mandatory.  That way there are less code paths to worry about.
> > > 
> > > I think Kevin requested this though I don't really remember the use case.
> > 
> > There is no legacy syntax for putting a throttle node anywhere but at
> > the root of a BlockBackend. If you want to throttle e.g. only a specific
> > backing file, you need to manually create a throttle node.
> > 
> > (We tend to forget this occasionally, but the work you're doing is not
> > only cleanup just for fun, but it's actually new features that enable
> > new use cases by making everything more flexible.)
> 
> It's not clear to me from your reply whether you support anonymous
> throttle groups or not.  It is possible to throttle arbitrary nodes in
> the graph either way.

I think it would be nice for human users to have them, but on second
thought you're right that it's just syntactic sugar for an explicit
-object, so if you think there is any difficulty with supporting
anonymous groups, feel free to drop them.

Hm, actually... Does this mean that then the whole 'limits' option in
the throttle driver could go away, with all of the parsing code, and the
group name becomes mandatory? That certainly does look tempting.

Kevin


signature.asc
Description: PGP signature


Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-03 Thread Stefan Hajnoczi
On Thu, Aug 03, 2017 at 10:08:01AM +0200, Kevin Wolf wrote:
> Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> > On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > > throttle block filter drive easy creation and configuration of 
> > > > > > throttle
> > > > > > groups in QMP and cli.
> > > > > >
> > > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > > struct for all throttle configuration needs in QMP.
> > > > > >
> > > > > > ThrottleGroups can be created via CLI as
> > > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > > where x-* are individual limit properties. Since we can't add 
> > > > > > non-scalar
> > > > > > properties in -object this interface must be used instead. However,
> > > > > > setting these properties must be disabled after initialization 
> > > > > > because
> > > > > > certain combinations of limits are forbidden and thus configuration
> > > > > > changes should be done in one transaction. The individual properties
> > > > > > will go away when support for non-scalar values in CLI is 
> > > > > > implemented
> > > > > > and thus are marked as experimental.
> > > > > >
> > > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > > ThrottleLimits
> > > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > > configuration in existing groups as follows:
> > > > > >
> > > > > > { "execute": "object-add",
> > > > > >   "arguments": {
> > > > > > "qom-type": "throttle-group",
> > > > > > "id": "foo",
> > > > > > "props" : {
> > > > > >   "limits": {
> > > > > >   "iops-total": 100
> > > > > >   }
> > > > > > }
> > > > > >   }
> > > > > > }
> > > > > > { "execute" : "qom-set",
> > > > > > "arguments" : {
> > > > > > "path" : "foo",
> > > > > > "property" : "limits",
> > > > > > "value" : {
> > > > > > "iops-total" : 99
> > > > > > }
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > This also means a group's configuration can be fetched with qom-get.
> > > > > >
> > > > > > ThrottleGroups can be anonymous which means they can't get accessed 
> > > > > > by
> > > > > > other users ie they will always be units instead of group (Because 
> > > > > > they
> > > > > > have one ThrottleGroupMember).
> > > > >
> > > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > > throttling.group= wasn't given.  So who will use anonymous 
> > > > > single-drive
> > > > > mode?
> > > > 
> > > > Manual filter nodes. It's possible to not pass a group name value and 
> > > > the
> > > > resulting group will be anonymous. Are you suggesting to move this 
> > > > change to
> > > > the throttle filter patch?
> > > 
> > > What is the use case?  Human users will stick to the legacy syntax
> > > because it's convenient.  Management tools will use the filter
> > > explicitly in the future, and it's easy for them to choose a name.
> > > 
> > > Unless there is a need for this case I'd prefer to make the group name
> > > mandatory.  That way there are less code paths to worry about.
> > 
> > I think Kevin requested this though I don't really remember the use case.
> 
> There is no legacy syntax for putting a throttle node anywhere but at
> the root of a BlockBackend. If you want to throttle e.g. only a specific
> backing file, you need to manually create a throttle node.
> 
> (We tend to forget this occasionally, but the work you're doing is not
> only cleanup just for fun, but it's actually new features that enable
> new use cases by making everything more flexible.)

It's not clear to me from your reply whether you support anonymous
throttle groups or not.  It is possible to throttle arbitrary nodes in
the graph either way.

Stefan


signature.asc
Description: PGP signature


Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-03 Thread Kevin Wolf
Am 02.08.2017 um 12:57 hat Manos Pitsidianakis geschrieben:
> On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > throttle block filter drive easy creation and configuration of 
> > > > > throttle
> > > > > groups in QMP and cli.
> > > > >
> > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > struct for all throttle configuration needs in QMP.
> > > > >
> > > > > ThrottleGroups can be created via CLI as
> > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > where x-* are individual limit properties. Since we can't add 
> > > > > non-scalar
> > > > > properties in -object this interface must be used instead. However,
> > > > > setting these properties must be disabled after initialization because
> > > > > certain combinations of limits are forbidden and thus configuration
> > > > > changes should be done in one transaction. The individual properties
> > > > > will go away when support for non-scalar values in CLI is implemented
> > > > > and thus are marked as experimental.
> > > > >
> > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > ThrottleLimits
> > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > configuration in existing groups as follows:
> > > > >
> > > > > { "execute": "object-add",
> > > > >   "arguments": {
> > > > > "qom-type": "throttle-group",
> > > > > "id": "foo",
> > > > > "props" : {
> > > > >   "limits": {
> > > > >   "iops-total": 100
> > > > >   }
> > > > > }
> > > > >   }
> > > > > }
> > > > > { "execute" : "qom-set",
> > > > > "arguments" : {
> > > > > "path" : "foo",
> > > > > "property" : "limits",
> > > > > "value" : {
> > > > > "iops-total" : 99
> > > > > }
> > > > > }
> > > > > }
> > > > >
> > > > > This also means a group's configuration can be fetched with qom-get.
> > > > >
> > > > > ThrottleGroups can be anonymous which means they can't get accessed by
> > > > > other users ie they will always be units instead of group (Because 
> > > > > they
> > > > > have one ThrottleGroupMember).
> > > >
> > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > throttling.group= wasn't given.  So who will use anonymous single-drive
> > > > mode?
> > > 
> > > Manual filter nodes. It's possible to not pass a group name value and the
> > > resulting group will be anonymous. Are you suggesting to move this change 
> > > to
> > > the throttle filter patch?
> > 
> > What is the use case?  Human users will stick to the legacy syntax
> > because it's convenient.  Management tools will use the filter
> > explicitly in the future, and it's easy for them to choose a name.
> > 
> > Unless there is a need for this case I'd prefer to make the group name
> > mandatory.  That way there are less code paths to worry about.
> 
> I think Kevin requested this though I don't really remember the use case.

There is no legacy syntax for putting a throttle node anywhere but at
the root of a BlockBackend. If you want to throttle e.g. only a specific
backing file, you need to manually create a throttle node.

(We tend to forget this occasionally, but the work you're doing is not
only cleanup just for fun, but it's actually new features that enable
new use cases by making everything more flexible.)

Kevin


signature.asc
Description: PGP signature


Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-02 Thread Stefan Hajnoczi
On Wed, Aug 02, 2017 at 01:57:04PM +0300, Manos Pitsidianakis wrote:
> On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:
> > On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> > > On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > > > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > > > ThrottleGroup is converted to an object. This will allow the future
> > > > > throttle block filter drive easy creation and configuration of 
> > > > > throttle
> > > > > groups in QMP and cli.
> > > > >
> > > > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > > > struct for all throttle configuration needs in QMP.
> > > > >
> > > > > ThrottleGroups can be created via CLI as
> > > > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > > > where x-* are individual limit properties. Since we can't add 
> > > > > non-scalar
> > > > > properties in -object this interface must be used instead. However,
> > > > > setting these properties must be disabled after initialization because
> > > > > certain combinations of limits are forbidden and thus configuration
> > > > > changes should be done in one transaction. The individual properties
> > > > > will go away when support for non-scalar values in CLI is implemented
> > > > > and thus are marked as experimental.
> > > > >
> > > > > ThrottleGroup also has a `limits` property that uses the 
> > > > > ThrottleLimits
> > > > > struct.  It can be used to create ThrottleGroups or set the
> > > > > configuration in existing groups as follows:
> > > > >
> > > > > { "execute": "object-add",
> > > > >   "arguments": {
> > > > > "qom-type": "throttle-group",
> > > > > "id": "foo",
> > > > > "props" : {
> > > > >   "limits": {
> > > > >   "iops-total": 100
> > > > >   }
> > > > > }
> > > > >   }
> > > > > }
> > > > > { "execute" : "qom-set",
> > > > > "arguments" : {
> > > > > "path" : "foo",
> > > > > "property" : "limits",
> > > > > "value" : {
> > > > > "iops-total" : 99
> > > > > }
> > > > > }
> > > > > }
> > > > >
> > > > > This also means a group's configuration can be fetched with qom-get.
> > > > >
> > > > > ThrottleGroups can be anonymous which means they can't get accessed by
> > > > > other users ie they will always be units instead of group (Because 
> > > > > they
> > > > > have one ThrottleGroupMember).
> > > >
> > > > blockdev.c automatically assigns -drive id= to the group name if
> > > > throttling.group= wasn't given.  So who will use anonymous single-drive
> > > > mode?
> > > 
> > > Manual filter nodes. It's possible to not pass a group name value and the
> > > resulting group will be anonymous. Are you suggesting to move this change 
> > > to
> > > the throttle filter patch?
> > 
> > What is the use case?  Human users will stick to the legacy syntax
> > because it's convenient.  Management tools will use the filter
> > explicitly in the future, and it's easy for them to choose a name.
> > 
> > Unless there is a need for this case I'd prefer to make the group name
> > mandatory.  That way there are less code paths to worry about.
> 
> I think Kevin requested this though I don't really remember the use case.

Kevin: Do you still want this?

> > 
> > > > > @@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char 
> > > > > *name)
> > > > >
> > > > >  qemu_mutex_lock(_groups_lock);
> > > > >
> > > > > -/* Look for an existing group with that name */
> > > > > -QTAILQ_FOREACH(iter, _groups, list) {
> > > > > -if (!strcmp(name, iter->name)) {
> > > > > -tg = iter;
> > > > > -break;
> > > > > +if (name) {
> > > > > +/* Look for an existing group with that name */
> > > > > +QTAILQ_FOREACH(iter, _groups, list) {
> > > > > +if (!g_strcmp0(name, iter->name)) {
> > > > > +tg = iter;
> > > > > +break;
> > > > > +}
> > > > >  }
> > > > >  }
> > > > >
> > > > >  /* Create a new one if not found */
> > > > >  if (!tg) {
> > > > > -tg = g_new0(ThrottleGroup, 1);
> > > > > +/* new ThrottleGroup obj will have a refcnt = 1 */
> > > > > +tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
> > > > >  tg->name = g_strdup(name);
> > > > > -tg->clock_type = QEMU_CLOCK_REALTIME;
> > > > > -
> > > > > -if (qtest_enabled()) {
> > > > > -/* For testing block IO throttling only */
> > > > > -tg->clock_type = QEMU_CLOCK_VIRTUAL;
> > > > > -}
> > > > > -qemu_mutex_init(>lock);
> > > > > -throttle_init(>ts);
> > > > > -QLIST_INIT(>head);
> > > > > -
> > > > > -QTAILQ_INSERT_TAIL(_groups, tg, list);
> > > > > +throttle_group_obj_complete((UserCreatable *)tg, 
> > > > > _abort);
> > > > >  }
> > > > >
> > > > > -tg->refcount++;
> > 

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-02 Thread Manos Pitsidianakis

On Wed, Aug 02, 2017 at 11:39:22AM +0100, Stefan Hajnoczi wrote:

On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:

On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > ThrottleGroup is converted to an object. This will allow the future
> > throttle block filter drive easy creation and configuration of throttle
> > groups in QMP and cli.
> >
> > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > struct for all throttle configuration needs in QMP.
> >
> > ThrottleGroups can be created via CLI as
> > -object throttle-group,id=foo,x-iops-total=100,x-..
> > where x-* are individual limit properties. Since we can't add non-scalar
> > properties in -object this interface must be used instead. However,
> > setting these properties must be disabled after initialization because
> > certain combinations of limits are forbidden and thus configuration
> > changes should be done in one transaction. The individual properties
> > will go away when support for non-scalar values in CLI is implemented
> > and thus are marked as experimental.
> >
> > ThrottleGroup also has a `limits` property that uses the ThrottleLimits
> > struct.  It can be used to create ThrottleGroups or set the
> > configuration in existing groups as follows:
> >
> > { "execute": "object-add",
> >   "arguments": {
> > "qom-type": "throttle-group",
> > "id": "foo",
> > "props" : {
> >   "limits": {
> >   "iops-total": 100
> >   }
> > }
> >   }
> > }
> > { "execute" : "qom-set",
> > "arguments" : {
> > "path" : "foo",
> > "property" : "limits",
> > "value" : {
> > "iops-total" : 99
> > }
> > }
> > }
> >
> > This also means a group's configuration can be fetched with qom-get.
> >
> > ThrottleGroups can be anonymous which means they can't get accessed by
> > other users ie they will always be units instead of group (Because they
> > have one ThrottleGroupMember).
>
> blockdev.c automatically assigns -drive id= to the group name if
> throttling.group= wasn't given.  So who will use anonymous single-drive
> mode?

Manual filter nodes. It's possible to not pass a group name value and the
resulting group will be anonymous. Are you suggesting to move this change to
the throttle filter patch?


What is the use case?  Human users will stick to the legacy syntax
because it's convenient.  Management tools will use the filter
explicitly in the future, and it's easy for them to choose a name.

Unless there is a need for this case I'd prefer to make the group name
mandatory.  That way there are less code paths to worry about.


I think Kevin requested this though I don't really remember the use 
case.



> > @@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char *name)
> >
> >  qemu_mutex_lock(_groups_lock);
> >
> > -/* Look for an existing group with that name */
> > -QTAILQ_FOREACH(iter, _groups, list) {
> > -if (!strcmp(name, iter->name)) {
> > -tg = iter;
> > -break;
> > +if (name) {
> > +/* Look for an existing group with that name */
> > +QTAILQ_FOREACH(iter, _groups, list) {
> > +if (!g_strcmp0(name, iter->name)) {
> > +tg = iter;
> > +break;
> > +}
> >  }
> >  }
> >
> >  /* Create a new one if not found */
> >  if (!tg) {
> > -tg = g_new0(ThrottleGroup, 1);
> > +/* new ThrottleGroup obj will have a refcnt = 1 */
> > +tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
> >  tg->name = g_strdup(name);
> > -tg->clock_type = QEMU_CLOCK_REALTIME;
> > -
> > -if (qtest_enabled()) {
> > -/* For testing block IO throttling only */
> > -tg->clock_type = QEMU_CLOCK_VIRTUAL;
> > -}
> > -qemu_mutex_init(>lock);
> > -throttle_init(>ts);
> > -QLIST_INIT(>head);
> > -
> > -QTAILQ_INSERT_TAIL(_groups, tg, list);
> > +throttle_group_obj_complete((UserCreatable *)tg, _abort);
> >  }
> >
> > -tg->refcount++;
> > +qemu_mutex_lock(>lock);
> > +if (!QLIST_EMPTY(>head)) {
> > +/* only ref if the group is not empty */
> > +object_ref(OBJECT(tg));
> > +}
> > +qemu_mutex_unlock(>lock);
>
> How do the refcounts work in various cases (legacy -drive
> throttling.group and -object throttle-group with 0, 1, or multiple
> drives)?
>
> It's not obvious to me that this code works in all cases.

Object is created with object_new(): ref count is 1
Each time we call throttle_group_incref() to add a new member to the group,
we increase the ref count by 1. We skip the first time we do that because
there's already a reference. When all members are removed, object is
deleted.


If the ThrottleGroup was created with -object throttle-group it
shouldn't disappear when the last 

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-02 Thread Stefan Hajnoczi
On Tue, Aug 01, 2017 at 07:49:33PM +0300, Manos Pitsidianakis wrote:
> On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:
> > On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> > > ThrottleGroup is converted to an object. This will allow the future
> > > throttle block filter drive easy creation and configuration of throttle
> > > groups in QMP and cli.
> > > 
> > > A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> > > struct for all throttle configuration needs in QMP.
> > > 
> > > ThrottleGroups can be created via CLI as
> > > -object throttle-group,id=foo,x-iops-total=100,x-..
> > > where x-* are individual limit properties. Since we can't add non-scalar
> > > properties in -object this interface must be used instead. However,
> > > setting these properties must be disabled after initialization because
> > > certain combinations of limits are forbidden and thus configuration
> > > changes should be done in one transaction. The individual properties
> > > will go away when support for non-scalar values in CLI is implemented
> > > and thus are marked as experimental.
> > > 
> > > ThrottleGroup also has a `limits` property that uses the ThrottleLimits
> > > struct.  It can be used to create ThrottleGroups or set the
> > > configuration in existing groups as follows:
> > > 
> > > { "execute": "object-add",
> > >   "arguments": {
> > > "qom-type": "throttle-group",
> > > "id": "foo",
> > > "props" : {
> > >   "limits": {
> > >   "iops-total": 100
> > >   }
> > > }
> > >   }
> > > }
> > > { "execute" : "qom-set",
> > > "arguments" : {
> > > "path" : "foo",
> > > "property" : "limits",
> > > "value" : {
> > > "iops-total" : 99
> > > }
> > > }
> > > }
> > > 
> > > This also means a group's configuration can be fetched with qom-get.
> > > 
> > > ThrottleGroups can be anonymous which means they can't get accessed by
> > > other users ie they will always be units instead of group (Because they
> > > have one ThrottleGroupMember).
> > 
> > blockdev.c automatically assigns -drive id= to the group name if
> > throttling.group= wasn't given.  So who will use anonymous single-drive
> > mode?
> 
> Manual filter nodes. It's possible to not pass a group name value and the
> resulting group will be anonymous. Are you suggesting to move this change to
> the throttle filter patch?

What is the use case?  Human users will stick to the legacy syntax
because it's convenient.  Management tools will use the filter
explicitly in the future, and it's easy for them to choose a name.

Unless there is a need for this case I'd prefer to make the group name
mandatory.  That way there are less code paths to worry about.

> > > @@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char *name)
> > > 
> > >  qemu_mutex_lock(_groups_lock);
> > > 
> > > -/* Look for an existing group with that name */
> > > -QTAILQ_FOREACH(iter, _groups, list) {
> > > -if (!strcmp(name, iter->name)) {
> > > -tg = iter;
> > > -break;
> > > +if (name) {
> > > +/* Look for an existing group with that name */
> > > +QTAILQ_FOREACH(iter, _groups, list) {
> > > +if (!g_strcmp0(name, iter->name)) {
> > > +tg = iter;
> > > +break;
> > > +}
> > >  }
> > >  }
> > > 
> > >  /* Create a new one if not found */
> > >  if (!tg) {
> > > -tg = g_new0(ThrottleGroup, 1);
> > > +/* new ThrottleGroup obj will have a refcnt = 1 */
> > > +tg = THROTTLE_GROUP(object_new(TYPE_THROTTLE_GROUP));
> > >  tg->name = g_strdup(name);
> > > -tg->clock_type = QEMU_CLOCK_REALTIME;
> > > -
> > > -if (qtest_enabled()) {
> > > -/* For testing block IO throttling only */
> > > -tg->clock_type = QEMU_CLOCK_VIRTUAL;
> > > -}
> > > -qemu_mutex_init(>lock);
> > > -throttle_init(>ts);
> > > -QLIST_INIT(>head);
> > > -
> > > -QTAILQ_INSERT_TAIL(_groups, tg, list);
> > > +throttle_group_obj_complete((UserCreatable *)tg, _abort);
> > >  }
> > > 
> > > -tg->refcount++;
> > > +qemu_mutex_lock(>lock);
> > > +if (!QLIST_EMPTY(>head)) {
> > > +/* only ref if the group is not empty */
> > > +object_ref(OBJECT(tg));
> > > +}
> > > +qemu_mutex_unlock(>lock);
> > 
> > How do the refcounts work in various cases (legacy -drive
> > throttling.group and -object throttle-group with 0, 1, or multiple
> > drives)?
> > 
> > It's not obvious to me that this code works in all cases.
> 
> Object is created with object_new(): ref count is 1
> Each time we call throttle_group_incref() to add a new member to the group,
> we increase the ref count by 1. We skip the first time we do that because
> there's already a reference. When all members are removed, object is
> deleted.

If the 

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-01 Thread Manos Pitsidianakis

On Tue, Aug 01, 2017 at 04:47:03PM +0100, Stefan Hajnoczi wrote:

On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:

ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.

A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.

ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.

ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct.  It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:

{ "execute": "object-add",
  "arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
  "limits": {
  "iops-total": 100
  }
}
  }
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}

This also means a group's configuration can be fetched with qom-get.

ThrottleGroups can be anonymous which means they can't get accessed by
other users ie they will always be units instead of group (Because they
have one ThrottleGroupMember).


blockdev.c automatically assigns -drive id= to the group name if
throttling.group= wasn't given.  So who will use anonymous single-drive
mode?


Manual filter nodes. It's possible to not pass a group name value and 
the resulting group will be anonymous. Are you suggesting to move this 
change to the throttle filter patch?



Signed-off-by: Manos Pitsidianakis 
---
Notes:
Note: I tested Markus Armbruster's patch in 
<87wp7fghi9@dusky.pond.sub.org>
on master and I can use this syntax successfuly:
-object '{ "qom-type" : "throttle-group", "id" : "foo", "props" : { 
"limits" \
: { "iops-total" : 1000 } } }'
If this gets merged using -object will be a little more verbose but at 
least we
won't have seperate properties, which is a good thing, so the x-* should be
dropped.

 block/throttle-groups.c | 402 
 include/block/throttle-groups.h |   3 +
 include/qemu/throttle-options.h |  59 --
 include/qemu/throttle.h |   3 +
 qapi/block-core.json|  48 +
 tests/test-throttle.c   |   1 +
 util/throttle.c | 151 +++
 7 files changed, 617 insertions(+), 50 deletions(-)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index f711a3dc62..b9c5036e44 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -25,9 +25,17 @@
 #include "qemu/osdep.h"
 #include "sysemu/block-backend.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 #include "sysemu/qtest.h"
+#include "qapi/error.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+#include "qom/object_interfaces.h"
+
+static void throttle_group_obj_init(Object *obj);
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);

 /* The ThrottleGroup structure (with its ThrottleState) is shared
  * among different ThrottleGroupMembers and it's independent from
@@ -54,6 +62,10 @@
  * that ThrottleGroupMember has throttled requests in the queue.
  */
 typedef struct ThrottleGroup {
+Object parent_obj;
+
+/* refuse individual property change if initialization is complete */
+bool is_initialized;
 char *name; /* This is constant during the lifetime of the group */

 QemuMutex lock; /* This lock protects the following four fields */
@@ -63,8 +75,7 @@ typedef struct ThrottleGroup {
 bool any_timer_armed[2];
 QEMUClockType clock_type;

-/* These two are protected by the global throttle_groups_lock */
-unsigned refcount;
+/* This is protected by the global throttle_groups_lock */
 QTAILQ_ENTRY(ThrottleGroup) list;
 } ThrottleGroup;

@@ -77,7 +88,8 @@ static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
  * If no ThrottleGroup is found with the given name a new one is
  * created.
  *
- * @name: the name of the ThrottleGroup
+ * @name: the name of the ThrottleGroup, NULL means a new anonymous group will
+ *be created.
  * @ret:  the ThrottleState member of the ThrottleGroup
  */
 ThrottleState *throttle_group_incref(const char *name)
@@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char *name)

 qemu_mutex_lock(_groups_lock);


Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

2017-08-01 Thread Stefan Hajnoczi
On Mon, Jul 31, 2017 at 12:54:40PM +0300, Manos Pitsidianakis wrote:
> ThrottleGroup is converted to an object. This will allow the future
> throttle block filter drive easy creation and configuration of throttle
> groups in QMP and cli.
> 
> A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> struct for all throttle configuration needs in QMP.
> 
> ThrottleGroups can be created via CLI as
> -object throttle-group,id=foo,x-iops-total=100,x-..
> where x-* are individual limit properties. Since we can't add non-scalar
> properties in -object this interface must be used instead. However,
> setting these properties must be disabled after initialization because
> certain combinations of limits are forbidden and thus configuration
> changes should be done in one transaction. The individual properties
> will go away when support for non-scalar values in CLI is implemented
> and thus are marked as experimental.
> 
> ThrottleGroup also has a `limits` property that uses the ThrottleLimits
> struct.  It can be used to create ThrottleGroups or set the
> configuration in existing groups as follows:
> 
> { "execute": "object-add",
>   "arguments": {
> "qom-type": "throttle-group",
> "id": "foo",
> "props" : {
>   "limits": {
>   "iops-total": 100
>   }
> }
>   }
> }
> { "execute" : "qom-set",
> "arguments" : {
> "path" : "foo",
> "property" : "limits",
> "value" : {
> "iops-total" : 99
> }
> }
> }
> 
> This also means a group's configuration can be fetched with qom-get.
> 
> ThrottleGroups can be anonymous which means they can't get accessed by
> other users ie they will always be units instead of group (Because they
> have one ThrottleGroupMember).

blockdev.c automatically assigns -drive id= to the group name if
throttling.group= wasn't given.  So who will use anonymous single-drive
mode?

> Signed-off-by: Manos Pitsidianakis 
> ---
> Notes:
> Note: I tested Markus Armbruster's patch in 
> <87wp7fghi9@dusky.pond.sub.org>
> on master and I can use this syntax successfuly:
> -object '{ "qom-type" : "throttle-group", "id" : "foo", "props" : { 
> "limits" \
> : { "iops-total" : 1000 } } }'
> If this gets merged using -object will be a little more verbose but at 
> least we
> won't have seperate properties, which is a good thing, so the x-* should 
> be
> dropped.
> 
>  block/throttle-groups.c | 402 
> 
>  include/block/throttle-groups.h |   3 +
>  include/qemu/throttle-options.h |  59 --
>  include/qemu/throttle.h |   3 +
>  qapi/block-core.json|  48 +
>  tests/test-throttle.c   |   1 +
>  util/throttle.c | 151 +++
>  7 files changed, 617 insertions(+), 50 deletions(-)
> 
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index f711a3dc62..b9c5036e44 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -25,9 +25,17 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/block-backend.h"
>  #include "block/throttle-groups.h"
> +#include "qemu/throttle-options.h"
>  #include "qemu/queue.h"
>  #include "qemu/thread.h"
>  #include "sysemu/qtest.h"
> +#include "qapi/error.h"
> +#include "qapi-visit.h"
> +#include "qom/object.h"
> +#include "qom/object_interfaces.h"
> +
> +static void throttle_group_obj_init(Object *obj);
> +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
>  
>  /* The ThrottleGroup structure (with its ThrottleState) is shared
>   * among different ThrottleGroupMembers and it's independent from
> @@ -54,6 +62,10 @@
>   * that ThrottleGroupMember has throttled requests in the queue.
>   */
>  typedef struct ThrottleGroup {
> +Object parent_obj;
> +
> +/* refuse individual property change if initialization is complete */
> +bool is_initialized;
>  char *name; /* This is constant during the lifetime of the group */
>  
>  QemuMutex lock; /* This lock protects the following four fields */
> @@ -63,8 +75,7 @@ typedef struct ThrottleGroup {
>  bool any_timer_armed[2];
>  QEMUClockType clock_type;
>  
> -/* These two are protected by the global throttle_groups_lock */
> -unsigned refcount;
> +/* This is protected by the global throttle_groups_lock */
>  QTAILQ_ENTRY(ThrottleGroup) list;
>  } ThrottleGroup;
>  
> @@ -77,7 +88,8 @@ static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
>   * If no ThrottleGroup is found with the given name a new one is
>   * created.
>   *
> - * @name: the name of the ThrottleGroup
> + * @name: the name of the ThrottleGroup, NULL means a new anonymous group 
> will
> + *be created.
>   * @ret:  the ThrottleState member of the ThrottleGroup
>   */
>  ThrottleState *throttle_group_incref(const char *name)
> @@ -87,32 +99,30 @@ ThrottleState *throttle_group_incref(const char *name)
>  
>