From: Vineeth Pillai <viremana@linux.microsoft.com>
These helper methods are used to capture vcpu information
in ch driver.
Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com>
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
---
src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++
src/util/virprocess.h | 5 ++
2 files changed, 141 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 6de3f36f52..0164d70df6 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED,
}
#endif /* !WITH_SCHED_SETSCHEDULER */
+
+/*
+TODO: This method was cloned from qemuGetProcessInfo in src/qemu/qemu_driver.c.
+Need to refactor qemu driver to use this shared function.
+*/
+int
+virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
+ pid_t pid, pid_t tid)
+{
+ g_autofree char *proc = NULL;
+ FILE *pidinfo;
+ unsigned long long usertime = 0, systime = 0;
+ long rss = 0;
+ int cpu = 0;
+
+ /* In general, we cannot assume pid_t fits in int; but /proc parsing
+ * is specific to Linux where int works fine. */
+ if (tid)
+ proc = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid, tid);
+ else
+ proc = g_strdup_printf("/proc/%d/stat", (int)pid);
+ if (!proc)
+ return -1;
+
+ pidinfo = fopen(proc, "r");
+
+ /* See 'man proc' for information about what all these fields are. We're
+ * only interested in a very few of them */
+ if (!pidinfo ||
+ fscanf(pidinfo,
+ /* pid -> stime */
+ "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu"
+ /* cutime -> endcode */
+ "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
+ /* startstack -> processor */
+ "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
+ &usertime, &systime, &rss, &cpu) != 4) {
+ VIR_WARN("cannot parse process status data");
+ }
+
+ /* We got jiffies
+ * We want nanoseconds
+ * _SC_CLK_TCK is jiffies per second
+ * So calculate thus....
+ */
+ if (cpuTime)
+ *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime)
+ / (unsigned long long)sysconf(_SC_CLK_TCK);
+ if (lastCpu)
+ *lastCpu = cpu;
+
+ if (vm_rss)
+ *vm_rss = rss * virGetSystemPageSizeKB();
+
+
+ VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
+ (int)pid, tid, usertime, systime, cpu, rss);
+
+ VIR_FORCE_FCLOSE(pidinfo);
+
+ return 0;
+}
+/*
+TODO: This method was cloned from qemuGetSchedInfo in src/qemu/qemu_driver.c.
+Need to refactor qemu driver to use this shared function.
+*/
+int virProcessGetSchedInfo(unsigned long long *cpuWait, pid_t pid, pid_t tid)
+{
+ g_autofree char *proc = NULL;
+ g_autofree char *data = NULL;
+ char **lines = NULL;
+ size_t i;
+ int ret = -1;
+ double val;
+
+ *cpuWait = 0;
+
+ /* In general, we cannot assume pid_t fits in int; but /proc parsing
+ * is specific to Linux where int works fine. */
+ if (tid)
+ proc = g_strdup_printf("/proc/%d/task/%d/sched", (int)pid, (int)tid);
+ else
+ proc = g_strdup_printf("/proc/%d/sched", (int)pid);
+ if (!proc)
+ goto cleanup;
+ ret = -1;
+
+ /* The file is not guaranteed to exist (needs CONFIG_SCHED_DEBUG) */
+ if (access(proc, R_OK) < 0) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (virFileReadAll(proc, (1<<16), &data) < 0)
+ goto cleanup;
+
+ lines = g_strsplit(data, "\n", 0);
+ if (!lines)
+ goto cleanup;
+
+ for (i = 0; lines[i] != NULL; i++) {
+ const char *line = lines[i];
+
+ /* Needs CONFIG_SCHEDSTATS. The second check
+ * is the old name the kernel used in past */
+ if (STRPREFIX(line, "se.statistics.wait_sum") ||
+ STRPREFIX(line, "se.wait_sum")) {
+ line = strchr(line, ':');
+ if (!line) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Missing separator in sched info '%s'"),
+ lines[i]);
+ goto cleanup;
+ }
+ line++;
+ while (*line == ' ')
+ line++;
+
+ if (virStrToDouble(line, NULL, &val) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to parse sched info value '%s'"),
+ line);
+ goto cleanup;
+ }
+
+ *cpuWait = (unsigned long long)(val * 1000000);
+ break;
+ }
+ }
+
+ ret = 0;
+
+ cleanup:
+ g_strfreev(lines);
+ return ret;
+}
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 9910331a0c..3a7c4c2d64 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -127,3 +127,8 @@ typedef enum {
} virProcessNamespaceFlags;
int virProcessNamespaceAvailable(unsigned int ns);
+
+int virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu,
+ long *vm_rss, pid_t pid, pid_t tid);
+int virProcessGetSchedInfo(unsigned long long *cpuWait, pid_t pid,
+ pid_t tid);
--
2.27.0
On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
> From: Vineeth Pillai <viremana@linux.microsoft.com>
>
> These helper methods are used to capture vcpu information
> in ch driver.
>
> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com>
> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
> ---
> src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++
> src/util/virprocess.h | 5 ++
> 2 files changed, 141 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 6de3f36f52..0164d70df6 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED,
> }
>
> #endif /* !WITH_SCHED_SETSCHEDULER */
> +
> +/*
> +TODO: This method was cloned from qemuGetProcessInfo in src/qemu/qemu_driver.c.
> +Need to refactor qemu driver to use this shared function.
There's no harm in doing that in this patch. In fact, it's desired. You
can move a qemu function into src/util, rename it and fix all places
where it is called, all in one patch.
Also, please don't forget to export this function in
src/libvirt_private.syms.
> +*/
> +int
> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
> + pid_t pid, pid_t tid)
Indentation's off. I've noticed other patches in the series suffer from
mis-indentation too. Please fix that in another version.
> +{
> + g_autofree char *proc = NULL;
> + FILE *pidinfo;
> + unsigned long long usertime = 0, systime = 0;
I wonder whether we should take this opportunity and declare these two
variables at one line each.
> + long rss = 0;
> + int cpu = 0;
Michal
On 10/29/2021 7:31 AM, Michal Prívozník wrote:
> On 10/22/21 5:37 PM, Praveen K Paladugu wrote:
>> From: Vineeth Pillai <viremana@linux.microsoft.com>
>>
>> These helper methods are used to capture vcpu information
>> in ch driver.
>>
>> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com>
>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
>> ---
>> src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++
>> src/util/virprocess.h | 5 ++
>> 2 files changed, 141 insertions(+)
>>
>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>> index 6de3f36f52..0164d70df6 100644
>> --- a/src/util/virprocess.c
>> +++ b/src/util/virprocess.c
>> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED,
>> }
>>
>> #endif /* !WITH_SCHED_SETSCHEDULER */
>> +
>> +/*
>> +TODO: This method was cloned from qemuGetProcessInfo in src/qemu/qemu_driver.c.
>> +Need to refactor qemu driver to use this shared function.
>
> There's no harm in doing that in this patch. In fact, it's desired. You
> can move a qemu function into src/util, rename it and fix all places
> where it is called, all in one patch.
>
> Also, please don't forget to export this function in
> src/libvirt_private.syms.
>
I am working on this now. Will make this part of next version.
>> +*/
>> +int
>> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
>> + pid_t pid, pid_t tid)
>
> Indentation's off. I've noticed other patches in the series suffer from
> mis-indentation too. Please fix that in another version.
>
Regarding indentation, many of these patches add/modify existing files.
Running indent will modify sections of the files not relevant to the
code changes in this patch set.
Should I run "indent" either way? Or should I make all indentation
changes for all files as a single commit?
>> +{
>> + g_autofree char *proc = NULL;
>> + FILE *pidinfo;
>> + unsigned long long usertime = 0, systime = 0;
>
> I wonder whether we should take this opportunity and declare these two
> variables at one line each.
>
>> + long rss = 0;
>> + int cpu = 0;
>
> Michal
>
--
Regards,
Praveen K Paladugu
On Wed, Nov 10, 2021 at 02:49:57PM -0600, Praveen K Paladugu wrote: > > > On 10/29/2021 7:31 AM, Michal Prívozník wrote: > > On 10/22/21 5:37 PM, Praveen K Paladugu wrote: > > > From: Vineeth Pillai <viremana@linux.microsoft.com> > > > > > > These helper methods are used to capture vcpu information > > > in ch driver. > > > > > > Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com> > > > Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> > > > --- > > > src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++ > > > src/util/virprocess.h | 5 ++ > > > 2 files changed, 141 insertions(+) > > > > > > diff --git a/src/util/virprocess.c b/src/util/virprocess.c > > > index 6de3f36f52..0164d70df6 100644 > > > --- a/src/util/virprocess.c > > > +++ b/src/util/virprocess.c > > > @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, > > > } > > > #endif /* !WITH_SCHED_SETSCHEDULER */ > > > + > > > +/* > > > +TODO: This method was cloned from qemuGetProcessInfo in src/qemu/qemu_driver.c. > > > +Need to refactor qemu driver to use this shared function. > > > > There's no harm in doing that in this patch. In fact, it's desired. You > > can move a qemu function into src/util, rename it and fix all places > > where it is called, all in one patch. > > > > Also, please don't forget to export this function in > > src/libvirt_private.syms. > > > I am working on this now. Will make this part of next version. > > > +*/ > > > +int > > > +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, > > > + pid_t pid, pid_t tid) > > > > Indentation's off. I've noticed other patches in the series suffer from > > mis-indentation too. Please fix that in another version. > > > Regarding indentation, many of these patches add/modify existing files. > Running indent will modify sections of the files not relevant to the code > changes in this patch set. > Should I run "indent" either way? Or should I make all indentation changes > for all files as a single commit? If thre are whitespace bugs in existing code, then it is preferrable to fix them in a standalone commit, so it is easily distinguished from functional changes. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/11/2021 3:19 AM, Daniel P. Berrangé wrote: > On Wed, Nov 10, 2021 at 02:49:57PM -0600, Praveen K Paladugu wrote: >> >> >> On 10/29/2021 7:31 AM, Michal Prívozník wrote: >>> On 10/22/21 5:37 PM, Praveen K Paladugu wrote: >>>> From: Vineeth Pillai <viremana@linux.microsoft.com> >>>> >>>> These helper methods are used to capture vcpu information >>>> in ch driver. >>>> >>>> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com> >>>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> >>>> --- >>>> src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++ >>>> src/util/virprocess.h | 5 ++ >>>> 2 files changed, 141 insertions(+) >>>> >>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c >>>> index 6de3f36f52..0164d70df6 100644 >>>> --- a/src/util/virprocess.c >>>> +++ b/src/util/virprocess.c >>>> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, >>>> } >>>> #endif /* !WITH_SCHED_SETSCHEDULER */ >>>> + >>>> +/* >>>> +TODO: This method was cloned from qemuGetProcessInfo in src/qemu/qemu_driver.c. >>>> +Need to refactor qemu driver to use this shared function. >>> >>> There's no harm in doing that in this patch. In fact, it's desired. You >>> can move a qemu function into src/util, rename it and fix all places >>> where it is called, all in one patch. >>> >>> Also, please don't forget to export this function in >>> src/libvirt_private.syms. >>> >> I am working on this now. Will make this part of next version. >>>> +*/ >>>> +int >>>> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, >>>> + pid_t pid, pid_t tid) >>> >>> Indentation's off. I've noticed other patches in the series suffer from >>> mis-indentation too. Please fix that in another version. >>> >> Regarding indentation, many of these patches add/modify existing files. >> Running indent will modify sections of the files not relevant to the code >> changes in this patch set. >> Should I run "indent" either way? Or should I make all indentation changes >> for all files as a single commit? > > If thre are whitespace bugs in existing code, then it is preferrable to > fix them in a standalone commit, so it is easily distinguished from > functional changes. > > Regards, > Daniel > Make sense Daniel. If any of the existing files don't have correct indentation, I will fix the original file in a separate commit and add my changes on top. -- Regards, Praveen K Paladugu
On 11/10/21 9:49 PM, Praveen K Paladugu wrote: > > > On 10/29/2021 7:31 AM, Michal Prívozník wrote: >> On 10/22/21 5:37 PM, Praveen K Paladugu wrote: >>> From: Vineeth Pillai <viremana@linux.microsoft.com> >>> >>> These helper methods are used to capture vcpu information >>> in ch driver. >>> >>> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com> >>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> >>> --- >>> src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++ >>> src/util/virprocess.h | 5 ++ >>> 2 files changed, 141 insertions(+) >>> >>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c >>> index 6de3f36f52..0164d70df6 100644 >>> --- a/src/util/virprocess.c >>> +++ b/src/util/virprocess.c >>> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, >>> } >>> #endif /* !WITH_SCHED_SETSCHEDULER */ >>> + >>> +/* >>> +TODO: This method was cloned from qemuGetProcessInfo in >>> src/qemu/qemu_driver.c. >>> +Need to refactor qemu driver to use this shared function. >> >> There's no harm in doing that in this patch. In fact, it's desired. You >> can move a qemu function into src/util, rename it and fix all places >> where it is called, all in one patch. >> >> Also, please don't forget to export this function in >> src/libvirt_private.syms. >> > I am working on this now. Will make this part of next version. >>> +*/ >>> +int >>> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, >>> long *vm_rss, >>> + pid_t pid, pid_t tid) >> >> Indentation's off. I've noticed other patches in the series suffer from >> mis-indentation too. Please fix that in another version. >> > Regarding indentation, many of these patches add/modify existing files. > Running indent will modify sections of the files not relevant to the > code changes in this patch set. > Should I run "indent" either way? Or should I make all indentation > changes for all files as a single commit? We like to have one semantic change per commit. Therefore, if you are fixing indentation in a code that's unrelated (i.e. you are fixing indentation in a function you are not touching) then that's considered as another semantic change. What I was aiming at is that new code should be indented well from the beginning. That's obviously one semantic change and as such should be in one patch (i.e. new code that's well formatted). Have I answered your question or did I misunderstand? Michal
On 11/11/2021 3:25 AM, Michal Prívozník wrote: > On 11/10/21 9:49 PM, Praveen K Paladugu wrote: >> >> >> On 10/29/2021 7:31 AM, Michal Prívozník wrote: >>> On 10/22/21 5:37 PM, Praveen K Paladugu wrote: >>>> From: Vineeth Pillai <viremana@linux.microsoft.com> >>>> >>>> These helper methods are used to capture vcpu information >>>> in ch driver. >>>> >>>> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com> >>>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> >>>> --- >>>> src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++ >>>> src/util/virprocess.h | 5 ++ >>>> 2 files changed, 141 insertions(+) >>>> >>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c >>>> index 6de3f36f52..0164d70df6 100644 >>>> --- a/src/util/virprocess.c >>>> +++ b/src/util/virprocess.c >>>> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, >>>> } >>>> #endif /* !WITH_SCHED_SETSCHEDULER */ >>>> + >>>> +/* >>>> +TODO: This method was cloned from qemuGetProcessInfo in >>>> src/qemu/qemu_driver.c. >>>> +Need to refactor qemu driver to use this shared function. >>> >>> There's no harm in doing that in this patch. In fact, it's desired. You >>> can move a qemu function into src/util, rename it and fix all places >>> where it is called, all in one patch. >>> >>> Also, please don't forget to export this function in >>> src/libvirt_private.syms. >>> >> I am working on this now. Will make this part of next version. >>>> +*/ >>>> +int >>>> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, >>>> long *vm_rss, >>>> + pid_t pid, pid_t tid) >>> >>> Indentation's off. I've noticed other patches in the series suffer from >>> mis-indentation too. Please fix that in another version. >>> >> Regarding indentation, many of these patches add/modify existing files. >> Running indent will modify sections of the files not relevant to the >> code changes in this patch set. >> Should I run "indent" either way? Or should I make all indentation >> changes for all files as a single commit? > > We like to have one semantic change per commit. Therefore, if you are > fixing indentation in a code that's unrelated (i.e. you are fixing > indentation in a function you are not touching) then that's considered > as another semantic change. > > What I was aiming at is that new code should be indented well from the > beginning. That's obviously one semantic change and as such should be in > one patch (i.e. new code that's well formatted). > > Have I answered your question or did I misunderstand? > > Michal > Thanks Michal. I will keep this in mind for V2 of this patch set. -- Regards, Praveen K Paladugu
Hey Michal, Thanks for your review of this patch set. I am Out of Office for 2 weeks. I will send an updated patch set addressing all your comments, next week. -- Regards, Praveen K Paladugu
© 2016 - 2026 Red Hat, Inc.