Re: [libvirt] [PATCH v5 08/10] domain: Introduce virDomainIOThreadSchedDelId

On 04/27/2015 10:35 AM, Peter Krempa wrote: > On Fri, Apr 24, 2015 at 12:06:00 -0400, John Ferlan wrote: >> We're about to allow IOThreads to be deleted, but an iothreadid may be >> included in some domain thread sched, so add a new API to allow removing >> an iothread from some entry. >> >> Then during the writing of the threadsched data and an additional check >> to determine whether the bitmap is all clear before writing it out. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 20 ++++++++++++++++++++ >> src/conf/domain_conf.h | 1 + >> src/libvirt_private.syms | 1 + >> 3 files changed, 22 insertions(+) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 9d4c916..5f99fbd 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -17455,6 +17455,24 @@ virDomainIOThreadIDDel(virDomainDefPtr def, >> } >> } >> >> +void >> +virDomainIOThreadSchedDelId(virDomainDefPtr def, >> + unsigned int iothreadid) >> +{ >> + size_t i; >> + >> + if (!def->cputune.iothreadsched || !def->cputune.niothreadsched) >> + return; >> + >> + for (i = 0; i < def->cputune.niothreadsched; i++) { >> + if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) { >> + ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids, >> + iothreadid)); >> + return; >> + } > > This function will need to remove the bitmap from the array once it's > clear, as ... > hmm.. OK - I think the iothreadsched implementation was probably incomplete... Different issue though >> + } >> +} >> + >> virDomainPinDefPtr >> virDomainPinFind(virDomainPinDefPtr *def, >> int npin, >> @@ -20897,6 +20915,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, >> virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; >> char *ids = NULL; >> >> + if (virBitmapIsAllClear(sp->ids)) >> + continue; > > ... this check isn't enough not to oputput empty <cputune> element if > you removed the last iothread that would have any info that would > trigger cputune to be formatted. The chance to have such situation is > extremely slim, but possible. > >> if (!(ids = virBitmapFormat(sp->ids))) >> goto error; >> virBufferAsprintf(buf, "<iothreadsched iothreads='%s' scheduler='%s'", > > Peter > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 926a176..0b18720 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17481,6 +17481,11 @@ virDomainIOThreadSchedDelId(virDomainDefPtr def, if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) { ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids, iothreadid)); + if (virBitmapIsAllClear(def->cputune.iothreadsched[i].ids)) { + virBitmapFree(def->cputune.iothreadsched[i].ids); + VIR_DELETE_ELEMENT(def->cputune.iothreadsched, i, + def->cputune.niothreadsched); + } return; } } @@ -20926,8 +20931,6 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i]; char *ids = NULL; - if (virBitmapIsAllClear(sp->ids)) - continue; if (!(ids = virBitmapFormat(sp->ids))) goto error; virBufferAsprintf(buf, "<iothreadsched iothreads='%s' scheduler='%s'",