[libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group

Wang Huaqiang posted 18 patches 7 years, 3 months ago
There is a newer version of this series
[libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
Posted by Wang Huaqiang 7 years, 3 months ago
The code of adding PID to the allocation could be reused, refactor it
for later reuse.

Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
---
 src/util/virresctrl.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 1d0eb01..5fc8772 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -2390,26 +2390,21 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
 }
 
 
-int
-virResctrlAllocAddPID(virResctrlAllocPtr alloc,
-                      pid_t pid)
+static int
+virResctrlAddPID(const char *path,
+                 pid_t pid)
 {
     char *tasks = NULL;
     char *pidstr = NULL;
     int ret = 0;
 
-    /* If the allocation is empty, then it is impossible to add a PID to
-     * allocation  due to lacking of its 'tasks' file. Just return */
-    if (virResctrlAllocIsEmpty(alloc))
-        return 0;
-
-    if (!alloc->path) {
+    if (!path) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Cannot add pid to non-existing resctrl allocation"));
+                       _("Cannot add pid to non-existing resctrl group"));
         return -1;
     }
 
-    if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0)
+    if (virAsprintf(&tasks, "%s/tasks", path) < 0)
         return -1;
 
     if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0)
@@ -2431,6 +2426,19 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc,
 
 
 int
+virResctrlAllocAddPID(virResctrlAllocPtr alloc,
+                      pid_t pid)
+{
+    /* If allocation is empty, then no resctrl directory and the 'tasks' file
+     * exists, just return */
+    if (virResctrlAllocIsEmpty(alloc))
+        return 0;
+
+    return virResctrlAddPID(alloc->path, pid);
+}
+
+
+int
 virResctrlAllocRemove(virResctrlAllocPtr alloc)
 {
     int ret = 0;
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
Posted by John Ferlan 7 years, 3 months ago

On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> The code of adding PID to the allocation could be reused, refactor it
> for later reuse.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> ---
>  src/util/virresctrl.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
Posted by Huaqiang,Wang 7 years, 3 months ago

On 2018年11月05日 23:03, John Ferlan wrote:
>
> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>> The code of adding PID to the allocation could be reused, refactor it
>> for later reuse.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>> ---
>>   src/util/virresctrl.c | 30 +++++++++++++++++++-----------
>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John

Thanks for the review.
Huaqiang

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
Posted by John Ferlan 7 years, 3 months ago

On 11/6/18 3:41 AM, Huaqiang,Wang wrote:
> 
> 
> On 2018年11月05日 23:03, John Ferlan wrote:
>>
>> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>>> The code of adding PID to the allocation could be reused, refactor it
>>> for later reuse.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
>>> ---
>>>   src/util/virresctrl.c | 30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> John
> 
> Thanks for the review.
> Huaqiang
> 

While working through patch1 adjustments, I noted an extra space in a
comment, so I fixed it:

    /* If the allocation is empty, then it is impossible to add a PID to
     * allocation  due to lacking of its 'tasks' file. Just return */
                 ^^
Of course that resulted in a merge conflict in this patch where I (now)
note you changed the comment to:

    /* If allocation is empty, then no resctrl directory and the 'tasks'
file
     * exists, just return */

I'm going to restore the original comment; however, it made me stop and
think about future patch14 which used the *tasks (and a new local *pid
list) - perhaps you need to rethink the changes... Even patch1...

What's the use of altering the *tasks file at all then?  Fortunately it
seems it's not really used for much more than logging the tids that are
running the vcpu.

I'll hold off on pushing anything...  So far there's no real impact, but
you may decide that you need to 'design in' a way to handle this
default/system path/issue.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv7 05/18] util: Refactor code for adding PID to the resource group
Posted by Wang, Huaqiang 7 years, 2 months ago

> -----Original Message-----
> From: John Ferlan [mailto:jferlan@redhat.com]
> Sent: Tuesday, November 6, 2018 10:41 PM
> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
> Cc: Feng, Shaohe <shaohe.feng@intel.com>; bing.niu@intel.com; Ding, Jian-
> feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@intel.com>
> Subject: Re: [PATCHv7 05/18] util: Refactor code for adding PID to the resource
> group
> 
> 
> 
> On 11/6/18 3:41 AM, Huaqiang,Wang wrote:
> >
> >
> > On 2018年11月05日 23:03, John Ferlan wrote:
> >>
> >> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
> >>> The code of adding PID to the allocation could be reused, refactor
> >>> it for later reuse.
> >>>
> >>> Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com>
> >>> ---
> >>>   src/util/virresctrl.c | 30 +++++++++++++++++++-----------
> >>>   1 file changed, 19 insertions(+), 11 deletions(-)
> >>>
> >> Reviewed-by: John Ferlan <jferlan@redhat.com>
> >>
> >> John
> >
> > Thanks for the review.
> > Huaqiang
> >
> 
> While working through patch1 adjustments, I noted an extra space in a
> comment, so I fixed it:
> 
>     /* If the allocation is empty, then it is impossible to add a PID to
>      * allocation  due to lacking of its 'tasks' file. Just return */
>                  ^^
> Of course that resulted in a merge conflict in this patch where I (now) note you
> changed the comment to:
> 
>     /* If allocation is empty, then no resctrl directory and the 'tasks'
> file
>      * exists, just return */
> 
> I'm going to restore the original comment; however, it made me stop and think
> about future patch14 which used the *tasks (and a new local *pid
> list) - perhaps you need to rethink the changes... Even patch1...
> 
> What's the use of altering the *tasks file at all then?  Fortunately it seems it's not
> really used for much more than logging the tids that are running the vcpu.
> 
> I'll hold off on pushing anything...  So far there's no real impact, but you may
> decide that you need to 'design in' a way to handle this default/system
> path/issue.
> 

I'll send out my v8 patches today, and will be covered in my new series, please make a review.

Thanks for review.

> John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list