procstat: nest the threads in the cpuset libxo output

[antranigv_freebsd.am]

Summary

procstat --libxo=json,pretty cs pgrep your_favorite_program would
return the correct data, however without nesting per thread.

This patch adds a thread-level nesting for the libxo output, making
the output more parsable.

Spotted By: Call For Testing Production Users Call Meetup Group

[antranigv_freebsd.am]

Question: since this is a breaking change, should I increment the PROCSTAT_XO_VERSION integer?

Thanks.

[js]

In D54924#1255808, @antranigv_freebsd.am wrote:

Question: since this is a breaking change, should I increment the PROCSTAT_XO_VERSION integer?

Thanks.

I think so, yes.

[js]

Can you share before and after output?

[antranigv_freebsd.am]

In D54924#1256188, @js wrote:

Can you share before and after output?

indeed.

Here’s what it looks like now (text, json, xml)

root@devbsd-01:~ # procstat --libxo=text,pretty cs `pgrep ntp`
  PID    TID COMM                TDNAME              CPU CSID CPU MASK
61289 100353 ntpd                -                    -1    1 0-7
61289 100968 ntpd                -                    -1    1 0-7
root@devbsd-01:~ # procstat --libxo=json,pretty cs `pgrep ntp`
{
  "__version": "1", 
  "procstat": {
    "cs": {
      "61289": {
        "process_id": 61289,
        "thread_id": 100353,
        "command": "ntpd",
        "thread_name": "-",
        "cpu": -1,
        "cpu_set_id": 1,
        "cpu_set": "0-7",
        "process_id": 61289,
        "thread_id": 100968,
        "command": "ntpd",
        "thread_name": "-",
        "cpu": -1,
        "cpu_set_id": 1,
        "cpu_set": "0-7"
      }
    }
  }
}
root@devbsd-01:~ # procstat --libxo=xml,pretty cs `pgrep ntp`
<procstat version="1">
  <cs>
    <_61289>
      <process_id>61289</process_id>
      <thread_id>100353</thread_id>
      <command>ntpd</command>
      <thread_name>-</thread_name>
      <cpu>-1</cpu>
      <cpu_set_id>1</cpu_set_id>
      <cpu_set>0-7</cpu_set>
      <process_id>61289</process_id>
      <thread_id>100968</thread_id>
      <command>ntpd</command>
      <thread_name>-</thread_name>
      <cpu>-1</cpu>
      <cpu_set_id>1</cpu_set_id>
      <cpu_set>0-7</cpu_set>
    </_61289>
  </cs>
</procstat>

My patch makes it look like this

root@devbsd-01:~ # /usr/obj/usr/src/amd64.amd64/usr.bin/procstat/procstat --libxo=json,pretty cs `pgrep ntp`
{
  "__version": "1", 
  "procstat": {
    "cs": {
      "61289": {
        "threads": [
          {
            "process_id": 61289,
            "thread_id": 100353,
            "command": "ntpd",
            "thread_name": "-",
            "cpu": -1,
            "cpu_set_id": 1,
            "cpu_set": "0-7"
          },
          {
            "process_id": 61289,
            "thread_id": 100968,
            "command": "ntpd",
            "thread_name": "-",
            "cpu": -1,
            "cpu_set_id": 1,
            "cpu_set": "0-7"
          }
        ]
      }
    }
  }
}
root@devbsd-01:~ # /usr/obj/usr/src/amd64.amd64/usr.bin/procstat/procstat --libxo=xml,pretty cs `pgrep ntp`
<procstat version="1">
  <cs>
    <_61289>
      <threads>
        <process_id>61289</process_id>
        <thread_id>100353</thread_id>
        <command>ntpd</command>
        <thread_name>-</thread_name>
        <cpu>-1</cpu>
        <cpu_set_id>1</cpu_set_id>
        <cpu_set>0-7</cpu_set>
      </threads>
      <threads>
        <process_id>61289</process_id>
        <thread_id>100968</thread_id>
        <command>ntpd</command>
        <thread_name>-</thread_name>
        <cpu>-1</cpu>
        <cpu_set_id>1</cpu_set_id>
        <cpu_set>0-7</cpu_set>
      </threads>
    </_61289>
  </cs>
</procstat>

Which I still don’t like…

maybe instead if using <threads> I just set it to <_THREADID> similar to how currently it uses the PID as key?

[js]

In D54924#1257121, @antranigv_freebsd.am wrote:

Which I still don’t like…

maybe instead if using <threads> I just set it to <_THREADID> similar to how currently it uses the PID as key?

Sure, feel free to the thread ID. You may want to open a container instead of an instance in that case.

Your output also made me aware that the original JSON output is invalid(dup keys), so this patch is definitely needed. Do you know if we need to fix other procstat flags as well?

[antranigv_freebsd.am]

Use xo_open_container instead of xo_open_instance
Increase the PROCSTAT_XO_VERSION
Use threadid as key

[js]

LGTM. @asomers: What do you think?

[asomers]

@antranigv_freebsd.am I see a few warnings when I use libxo's warn option:

> sudo /usr/obj/usr/home/somers/src/freebsd.org/src/amd64.amd64/usr.bin/procstat/procstat  --libxo=json,pretty,warn cs (pgrep ntp)
procstat: invalid XML tag name: '98363'
{
  "__version": "2", 
  "procstat": {
    "cs": {
      "98363": {
procstat: invalid XML tag name: '100222'
procstat: key field emitted after normal value field: 'thread_id'
        "100222": {
          "process_id": 98363,
          "thread_id": 100222,
          "command": "ntpd",
          "thread_name": "-",
          "cpu": -1,
          "cpu_set_id": 1,
          "cpu_set": "0-15"procstat: invalid XML tag name: '100222'
procstat: invalid XML tag name: '98363'

        }
      }
    }
  }
}

And I see dozens of warnings with xolint:

somers@fbsd-head ~/s/f/s/u/procstat (arcpatch-D54924)> pwd
/usr/home/somers/src/freebsd.org/src/usr.bin/procstat
somers@fbsd-head ~/s/f/s/u/procstat (arcpatch-D54924)> xolint *
Makefile: 0 errors, 0 warnings, 0 info
Makefile.depend: 0 errors, 0 warnings, 0 info
procstat.1: 0 errors, 0 warnings, 0 info
procstat.c: 0 errors, 0 warnings, 0 info
procstat.h: 0 errors, 0 warnings, 0 info
procstat_advlock.c: 0 errors, 0 warnings, 0 info
procstat_args.c: 0 errors, 0 warnings, 0 info
procstat_auxv.c: 75: error: encoding format cannot be given when content is present
...

Can you address those?

[js]

Simplest solution (and maybe cleanest?) I can think of is to prepend tid_ or simply t_ to threadid?