Bug 431047 - Review Request: latencytop - System latency monitor
Summary: Review Request: latencytop - System latency monitor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-01-31 13:28 UTC by Michal Schmidt
Modified: 2009-01-28 07:03 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-07 01:52:23 UTC
Type: ---
Embargoed:
panemade: fedora-review+


Attachments (Terms of Use)

Description Michal Schmidt 2008-01-31 13:28:13 UTC
Spec URL: http://michich.fedorapeople.org/latencytop/latencytop.spec
SRPM URL: http://michich.fedorapeople.org/latencytop/latencytop-0.3-1.fc9.src.rpm
Description:
LatencyTOP is a tool for software developers (both kernel and userspace), aimed at identifying where in the system latency is happening, and what kind of operation/action is causing the latency to happen so that the code can be changed to avoid the worst latency hiccups.

Comment 1 Michal Schmidt 2008-01-31 13:31:23 UTC
The package builds successfully in mock on my x86_64 system.

$ rpmlint /var/lib/mock/fedora-development-x86_64/result/
latencytop.x86_64: W: no-documentation

This is expected. Upstream tarball contains no documentation.

Comment 2 Parag AN(पराग) 2008-01-31 14:43:41 UTC
The issue I see with this package on F8 machine when tried to run latencytop =>
Please enable the CONFIG_LATENCYTOP configuration in your kernel.
Exiting...

Is CONFIG_LATENCYTOP is enabled in newer builds of kernel. If not then how can
one test functionality of this package?

About Packaging issues =>
1)Better recreate latencytop-standard-cflags.patch that will remove unnecessary
space caught in last few lines of patch.
  
2) Try to ask upstream to include some README and COPYING file. If this will
take some time then ask them to include it in next release. 
Note :- This is not a blocker for review


Comment 3 Michal Schmidt 2008-01-31 15:02:03 UTC
CONFIG_LATENCYTOP is a very new feature merged into Linux after 2.6.24 was
released. Currently one has to run a self-built kernel from Linus's git tree to
use it.

I'll fix issue #1 and contact upstream about #2.

Comment 4 Parag AN(पराग) 2008-02-01 03:11:56 UTC
waiting for your updated SRPM

Comment 5 Michal Schmidt 2008-02-01 13:13:20 UTC
I updated the package:
http://michich.fedorapeople.org/latencytop/latencytop.spec
http://michich.fedorapeople.org/latencytop/latencytop-0.3-2.fc9.src.rpm

ad 1) latencytop-standard-cflags.patch now removes the extra whitespace.
ad 2) Arjan van de Ven agreed to provide the COPYING file in the future.

I noticed another problem with the program. Targeted SELinux policy denies write
access to /proc/*/{sched,latency} even to unconfined_t processes. I'll file a
bug in the policy and see what Dan Walsh thinks.

Comment 6 Michal Schmidt 2008-02-01 16:13:52 UTC
I filed bug 431221 for the SELinux policy issue.

Comment 7 Parag AN(पराग) 2008-02-04 04:29:51 UTC
(In reply to comment #5)
> I updated the package:
> http://michich.fedorapeople.org/latencytop/latencytop.spec
> http://michich.fedorapeople.org/latencytop/latencytop-0.3-2.fc9.src.rpm
> 
> ad 1) latencytop-standard-cflags.patch now removes the extra whitespace.
  Can't see this fixed in new SRPM.


Comment 8 Parag AN(पराग) 2008-02-04 04:41:45 UTC
Review:
+ package builds in mock (rawhide).
+ rpmlint on for SRPM and RPM.
latencytop.i386: W: no-documentation
+ source files match upstream.
3ed2878fb7772e2a500ec71aa01abbb2  latencytop-0.3.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in source code.
- %doc files NOT present.
+ BuildRequires are proper.
+ %clean is present.
+ package installed properly.
+ Compiler flags used correctly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no -devel subpackage exists.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets are used.
+ Not a GUI app.

SHHOULD:
  1)Better recreate latencytop-standard-cflags.patch that will remove
unnecessary space caught in last few lines of patch.
  2)Try to ask upstream to include some README where it should be written what
this tool does and what are its limitations like this works on kernel >= 2.6.24
  3) Hope to see upstream will include license file in next upstream release.

APPROVED.


Comment 9 Parag AN(पराग) 2008-02-04 04:44:18 UTC
Can you tell me how can I test this on rawhide?

Comment 10 Michal Schmidt 2008-02-04 12:19:13 UTC
re 1) We must have a misunderstanding about what whitespace you meant. I thought
you referred to the extra line. Maybe you wanted me to drop the second hunk of
the patch completely?

re 2 and 3) I agree it would be nice if upstream provided a README. I already
asked for one when I was asking for the COPYING file. We'll see if a new
upstream version adds them.


I can see Rawhide has kernel-2.6.24-9 currently. This kernel does not have
LATENCYTOP support yet. The patch was merged upstream after the release of
2.6.24 in git commit 9745512ce... "sched: latencytop support".
So you'd need to compile the kernel from the current git tree. Make sure you
have CONFIG_LATENCYTOP enabled. After boot, start the measurement with:
echo 1 > /proc/sys/kernel/latencytop
Then you can run latencytop and see the output similar to the screenshot at
http://www.latencytop.org/ .

Comment 11 Parag AN(पराग) 2008-02-04 12:29:00 UTC
(In reply to comment #10)
> re 1) We must have a misunderstanding about what whitespace you meant. I thought
> you referred to the extra line. Maybe you wanted me to drop the second hunk of
> the patch completely?

Yes. I mean following lines from patch to be removed
=============================================================================
 #
 # The w in -lncursesw is not a typo; it is the wide-character version
@@ -10,8 +10,7 @@ LDF = -Wl,--as-needed `pkg-config --libs
 # libncursesw5-dev package.
 #
 latencytop: latencytop.o display.o latencytop.h translate.o Makefile
-       gcc $(CFLAGS) latencytop.o display.o translate.o $(LDF)-o latencytop
-
+       gcc $(CFLAGS) latencytop.o display.o translate.o $(LDF) -o latencytop

 clean:
        rm -f *~ latencytop DEADJOE *.o
=========================================================================      
                                         
> 
> re 2 and 3) I agree it would be nice if upstream provided a README. I already
> asked for one when I was asking for the COPYING file. We'll see if a new
> upstream version adds them.
  Thanks.

> 
> 
> I can see Rawhide has kernel-2.6.24-9 currently. This kernel does not have
> LATENCYTOP support yet. The patch was merged upstream after the release of
> 2.6.24 in git commit 9745512ce... "sched: latencytop support".
> So you'd need to compile the kernel from the current git tree. Make sure you
> have CONFIG_LATENCYTOP enabled. After boot, start the measurement with:
> echo 1 > /proc/sys/kernel/latencytop
> Then you can run latencytop and see the output similar to the screenshot at
> http://www.latencytop.org/ .

 Hope to see our kernels will get compiled with CONFIG_LATENCYTOP enabled.

Comment 12 Michal Schmidt 2008-02-04 16:29:14 UTC
Alright, I removed the second part of the patch. Now it's just a one-liner
changing the CFLAGS.

Updated package:
http://michich.fedorapeople.org/latencytop/latencytop-0.3-3.fc9.src.rpm
http://michich.fedorapeople.org/latencytop/latencytop.spec

Rawhide kernels usually do have many debugging options enabled. I expect
LATENCYTOP will be enabled too.

Thank you for the review.

New Package CVS Request
=======================
Package Name: latencytop
Short Description: System latency monitor
Owners: michich
Branches: F-8
InitialCC:
Cvsextras Commits: yes


Comment 13 Kevin Fenzi 2008-02-04 19:35:58 UTC
cvs done.

Comment 14 Parag AN(पराग) 2008-02-07 01:52:23 UTC
Closing this as this package has been successfully built for all requested
branches on build server.

Comment 15 Jeremy West 2009-01-26 17:02:42 UTC
Package Change Request
======================
Package Name: latencytop
New Branches: EL-4 EL-5
Owners: michich

Comment 16 Michal Schmidt 2009-01-26 17:49:46 UTC
Jeremy,

please explain what you mean by the request. Are you willing to maintain the package in the new branches or do you expect me to do it?
I have doubts about the usefulness of latencytop in EL-4 and EL-5 because their kernel do not have latencytop support.


Note You need to log in before you can comment on or make changes to this bug.