mbox series

[v2,00/12] simpletrace: refactor and general improvements

Message ID 20230502092339.27341-1-mads@ynddal.dk (mailing list archive)
Headers show
Series simpletrace: refactor and general improvements | expand

Message

Mads Ynddal May 2, 2023, 9:23 a.m. UTC
From: Mads Ynddal <m.ynddal@samsung.com>

I wanted to use simpletrace.py for an internal project, so I tried to update
and polish the code. Some of the commits resolve specific issues, while some
are more subjective.

I've tried to divide it into commits so we can discuss the
individual changes, and I'm ready to pull things out, if it isn't needed.

v2:
 * Added myself as maintainer of simpletrace.py
 * Improve docstring on `process`
 * Changed call to `process` in scripts/analyse-locks-simpletrace.py to reflect new argument types
 * Replaced `iteritems()` with `items()` in scripts/analyse-locks-simpletrace.py to support Python 3

Mads Ynddal (12):
  simpletrace: Improve parsing of sys.argv; fix files never closed.
  simpletrace: Annotate magic constants from QEMU code
  simpletrace: changed naming of edict and idtoname to improve
    readability
  simpletrace: update code for Python 3.11
  simpletrace: Changed Analyzer class to become context-manager
  simpletrace: Simplify construction of tracing methods
  simpletrace: Improved error handling on struct unpack
  simpletrace: define exception and add handling
  simpletrace: Refactor to separate responsibilities
  MAINTAINERS: add maintainer of simpletrace.py
  scripts/analyse-locks-simpletrace.py: changed iteritems() to items()
  scripts/analyse-locks-simpletrace.py: reflect changes to process in
    simpletrace.py

 MAINTAINERS                          |   8 +-
 scripts/analyse-locks-simpletrace.py |   5 +-
 scripts/simpletrace.py               | 307 ++++++++++++---------------
 3 files changed, 150 insertions(+), 170 deletions(-)

Comments

John Snow May 3, 2023, 1:55 p.m. UTC | #1
On Tue, May 2, 2023, 5:24 AM Mads Ynddal <mads@ynddal.dk> wrote:

> From: Mads Ynddal <m.ynddal@samsung.com>
>
> I wanted to use simpletrace.py for an internal project, so I tried to
> update
> and polish the code. Some of the commits resolve specific issues, while
> some
> are more subjective.
>
> I've tried to divide it into commits so we can discuss the
> individual changes, and I'm ready to pull things out, if it isn't needed.
>

Just a quick note to say that I'll be on and off the rest of this week, but
this is on my radar to look at soon.

A question for you: do you think it's possible to move simpletrace into
qemu/python/utils? This requires cleaning up the code to some fairly
pedantic standards, but helps protect it against rot as we change target
python versions.

No problem if that's too much to ask. just want to know if you had looked
into it.


> v2:
>  * Added myself as maintainer of simpletrace.py
>  * Improve docstring on `process`
>  * Changed call to `process` in scripts/analyse-locks-simpletrace.py to
> reflect new argument types
>  * Replaced `iteritems()` with `items()` in
> scripts/analyse-locks-simpletrace.py to support Python 3
>
> Mads Ynddal (12):
>   simpletrace: Improve parsing of sys.argv; fix files never closed.
>   simpletrace: Annotate magic constants from QEMU code
>   simpletrace: changed naming of edict and idtoname to improve
>     readability
>   simpletrace: update code for Python 3.11
>   simpletrace: Changed Analyzer class to become context-manager
>   simpletrace: Simplify construction of tracing methods
>   simpletrace: Improved error handling on struct unpack
>   simpletrace: define exception and add handling
>   simpletrace: Refactor to separate responsibilities
>   MAINTAINERS: add maintainer of simpletrace.py
>   scripts/analyse-locks-simpletrace.py: changed iteritems() to items()
>   scripts/analyse-locks-simpletrace.py: reflect changes to process in
>     simpletrace.py
>
>  MAINTAINERS                          |   8 +-
>  scripts/analyse-locks-simpletrace.py |   5 +-
>  scripts/simpletrace.py               | 307 ++++++++++++---------------
>  3 files changed, 150 insertions(+), 170 deletions(-)
>
> --
> 2.38.1
>
>
>
Stefan Hajnoczi May 4, 2023, 5:48 p.m. UTC | #2
On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote:
> From: Mads Ynddal <m.ynddal@samsung.com>
> 
> I wanted to use simpletrace.py for an internal project, so I tried to update
> and polish the code. Some of the commits resolve specific issues, while some
> are more subjective.

An internal project based on qemu.git or a completely separate codebase?

Sometimes I've wondered whether tracetool should be extracted from
qemu.git and moved into its own QEMU-independent place. That way other
C/C++ applications and libraries could use it easily.

Now that Alex Bennee removed the vcpu trace events that were specific to
QEMU, the tracing code is less tightly coupled to QEMU. There are
probably still a number of places that need to be cleaned up in order
for the tracing code to be independent of QEMU though.

If there is interest in doing this, I support the effort, although I'm
not sure how much time I have to actually do the work myself.

> 
> I've tried to divide it into commits so we can discuss the
> individual changes, and I'm ready to pull things out, if it isn't needed.
> 
> v2:
>  * Added myself as maintainer of simpletrace.py
>  * Improve docstring on `process`
>  * Changed call to `process` in scripts/analyse-locks-simpletrace.py to reflect new argument types
>  * Replaced `iteritems()` with `items()` in scripts/analyse-locks-simpletrace.py to support Python 3
> 
> Mads Ynddal (12):
>   simpletrace: Improve parsing of sys.argv; fix files never closed.
>   simpletrace: Annotate magic constants from QEMU code
>   simpletrace: changed naming of edict and idtoname to improve
>     readability
>   simpletrace: update code for Python 3.11
>   simpletrace: Changed Analyzer class to become context-manager
>   simpletrace: Simplify construction of tracing methods
>   simpletrace: Improved error handling on struct unpack
>   simpletrace: define exception and add handling
>   simpletrace: Refactor to separate responsibilities
>   MAINTAINERS: add maintainer of simpletrace.py
>   scripts/analyse-locks-simpletrace.py: changed iteritems() to items()
>   scripts/analyse-locks-simpletrace.py: reflect changes to process in
>     simpletrace.py
> 
>  MAINTAINERS                          |   8 +-
>  scripts/analyse-locks-simpletrace.py |   5 +-
>  scripts/simpletrace.py               | 307 ++++++++++++---------------
>  3 files changed, 150 insertions(+), 170 deletions(-)
> 
> -- 
> 2.38.1
>
John Snow May 4, 2023, 5:53 p.m. UTC | #3
On Thu, May 4, 2023, 1:48 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote:
> > From: Mads Ynddal <m.ynddal@samsung.com>
> >
> > I wanted to use simpletrace.py for an internal project, so I tried to
> update
> > and polish the code. Some of the commits resolve specific issues, while
> some
> > are more subjective.
>
> An internal project based on qemu.git or a completely separate codebase?
>
> Sometimes I've wondered whether tracetool should be extracted from
> qemu.git and moved into its own QEMU-independent place. That way other
> C/C++ applications and libraries could use it easily.
>
> Now that Alex Bennee removed the vcpu trace events that were specific to
> QEMU, the tracing code is less tightly coupled to QEMU. There are
> probably still a number of places that need to be cleaned up in order
> for the tracing code to be independent of QEMU though.
>
> If there is interest in doing this, I support the effort, although I'm
> not sure how much time I have to actually do the work myself.
>

I meant internal, but if there's interest in fully extracting it, I have a
playbook for that now based on my efforts to do the same for qemu.qmp and I
can offer good stewardship for that process.

I only suggest moving it under python/ so that we have it type checked and
tested against python deprecations as part of CI as a preventative measure
against rot.

(Stuff in python/ is rigorously checked, stuff in scripts/ is not.)

--js


> >
> > I've tried to divide it into commits so we can discuss the
> > individual changes, and I'm ready to pull things out, if it isn't needed.
> >
> > v2:
> >  * Added myself as maintainer of simpletrace.py
> >  * Improve docstring on `process`
> >  * Changed call to `process` in scripts/analyse-locks-simpletrace.py to
> reflect new argument types
> >  * Replaced `iteritems()` with `items()` in
> scripts/analyse-locks-simpletrace.py to support Python 3
> >
> > Mads Ynddal (12):
> >   simpletrace: Improve parsing of sys.argv; fix files never closed.
> >   simpletrace: Annotate magic constants from QEMU code
> >   simpletrace: changed naming of edict and idtoname to improve
> >     readability
> >   simpletrace: update code for Python 3.11
> >   simpletrace: Changed Analyzer class to become context-manager
> >   simpletrace: Simplify construction of tracing methods
> >   simpletrace: Improved error handling on struct unpack
> >   simpletrace: define exception and add handling
> >   simpletrace: Refactor to separate responsibilities
> >   MAINTAINERS: add maintainer of simpletrace.py
> >   scripts/analyse-locks-simpletrace.py: changed iteritems() to items()
> >   scripts/analyse-locks-simpletrace.py: reflect changes to process in
> >     simpletrace.py
> >
> >  MAINTAINERS                          |   8 +-
> >  scripts/analyse-locks-simpletrace.py |   5 +-
> >  scripts/simpletrace.py               | 307 ++++++++++++---------------
> >  3 files changed, 150 insertions(+), 170 deletions(-)
> >
> > --
> > 2.38.1
> >
>
Mads Ynddal May 8, 2023, 1:28 p.m. UTC | #4
> A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. 
> 
> No problem if that's too much to ask. just want to know if you had looked into it.

Potentially, this would be possible. But `simpletrace.py` imports
`qemu/scripts/tracetool/`, so that would have to be moved as well, or installed
as a package. I haven't touched the `tracetool` code itself, so I'm not sure how
feasible it is (i.e. how many other places use `tracetool`).
Stefan Hajnoczi May 8, 2023, 3:07 p.m. UTC | #5
On Thu, May 04, 2023 at 01:53:38PM -0400, John Snow wrote:
> On Thu, May 4, 2023, 1:48 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote:
> > > From: Mads Ynddal <m.ynddal@samsung.com>
> > >
> > > I wanted to use simpletrace.py for an internal project, so I tried to
> > update
> > > and polish the code. Some of the commits resolve specific issues, while
> > some
> > > are more subjective.
> >
> > An internal project based on qemu.git or a completely separate codebase?
> >
> > Sometimes I've wondered whether tracetool should be extracted from
> > qemu.git and moved into its own QEMU-independent place. That way other
> > C/C++ applications and libraries could use it easily.
> >
> > Now that Alex Bennee removed the vcpu trace events that were specific to
> > QEMU, the tracing code is less tightly coupled to QEMU. There are
> > probably still a number of places that need to be cleaned up in order
> > for the tracing code to be independent of QEMU though.
> >
> > If there is interest in doing this, I support the effort, although I'm
> > not sure how much time I have to actually do the work myself.
> >
> 
> I meant internal, but if there's interest in fully extracting it, I have a
> playbook for that now based on my efforts to do the same for qemu.qmp and I
> can offer good stewardship for that process.

I was curious how Mads is using simpletrace for an internal (to
Samsung?) project.

Maybe only simpletrace.py is needed because Mads' own code emits trace
logs in the simpletrace binary format, but in the general case we could
extract most of QEMU's tracing infrastructure (including tracetool).

Stefan
Stefan Hajnoczi May 8, 2023, 3:16 p.m. UTC | #6
On Mon, May 08, 2023 at 03:28:04PM +0200, Mads Ynddal wrote:
> 
> > A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. 
> > 
> > No problem if that's too much to ask. just want to know if you had looked into it.
> 
> Potentially, this would be possible. But `simpletrace.py` imports
> `qemu/scripts/tracetool/`, so that would have to be moved as well, or installed
> as a package. I haven't touched the `tracetool` code itself, so I'm not sure how
> feasible it is (i.e. how many other places use `tracetool`).

tracetool is only used by QEMU's build system to generate code from the
trace-events files. In theory it's possible to move it.

I'm not sure I understand the purpose of moving it to python/. What
infrastructure does python/ provide that's not available to
simpletrace.py in its current location?

Stefan
Mads Ynddal May 8, 2023, 4:50 p.m. UTC | #7
> 
> I was curious how Mads is using simpletrace for an internal (to
> Samsung?) project.
> 

I was just tracing the NVMe emulation to get some metrics. The code is all
upstream or a part of this patchset. The rest is tracing configs.
Stefan Hajnoczi May 9, 2023, 2:33 p.m. UTC | #8
On Mon, May 08, 2023 at 06:50:58PM +0200, Mads Ynddal wrote:
> 
> > 
> > I was curious how Mads is using simpletrace for an internal (to
> > Samsung?) project.
> > 
> 
> I was just tracing the NVMe emulation to get some metrics. The code is all
> upstream or a part of this patchset. The rest is tracing configs.

I see, not a different codebase from QEMU. In that case what I said
about extracting tracetool and simpletrace from qemu.git won't be
useful.

Stefan
John Snow May 10, 2023, 7:14 p.m. UTC | #9
On Mon, May 8, 2023 at 11:16 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Mon, May 08, 2023 at 03:28:04PM +0200, Mads Ynddal wrote:
> >
> > > A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions.
> > >
> > > No problem if that's too much to ask. just want to know if you had looked into it.
> >
> > Potentially, this would be possible. But `simpletrace.py` imports
> > `qemu/scripts/tracetool/`, so that would have to be moved as well, or installed
> > as a package. I haven't touched the `tracetool` code itself, so I'm not sure how
> > feasible it is (i.e. how many other places use `tracetool`).
>
> tracetool is only used by QEMU's build system to generate code from the
> trace-events files. In theory it's possible to move it.
>
> I'm not sure I understand the purpose of moving it to python/. What
> infrastructure does python/ provide that's not available to
> simpletrace.py in its current location?
>
> Stefan

It's just directory-based:

python/qemu/ are formatted as packages, with python/qemu/util/ serving
as a package that houses a bag of scripts. You can install these
things as a package into a venv and use them anywhere.
code in python/ is checked with a linter, type checker, etc while code
in scripts/ is not. It isn't installed anywhere and you may or may not
have to carefully set up PYTHONPATH= or choose your CWD carefully to
get the imports correct.

Over time I want to move more python code over under python/ so it
gets checked with the robots. The best stuff to move is stuff with
imports and dependencies. Truly standalone scripts aren't as important
to move.

It's not important for today, so let's not worry about it for this series.

--js