Re: [PATCH][RFC] oprofile 2.5.38 patch

[Christoph Hellwig]

On Mon, Sep 23, 2002 at 11:29:33PM +0100, John Levon wrote:
> 
> At :
> 
> http://oprofile.sourceforge.net/oprofile-2.5.html

Comments:

+if [ "$CONFIG_EXPERIMENTAL" = "y" ]; then
+   mainmenu_option next_comment
+   comment 'Profiling support'
+   bool 'Profiling support' CONFIG_PROFILING
+   source drivers/oprofile/Config.in
+   endmenu
+fi

The check and the menu should go into drivers/oprofile/Config.in

+OProfile system profiling
+CONFIG_OPROFILE
+  OProfile is a profiling system capable of profiling the
+  whole system, include the kernel, kernel modules, libraries,
+  and applications.
+
+  If unsure, say N.

For 2.5 we don't need/want the 'OProfile system profiling' line.

if [ "$CONFIG_PREEMPT" != "y" ]; then
+   dep_tristate 'OProfile system profiling' CONFIG_OPROFILE
$CONFIG_EXPERIMENTAL $CONFIG_PROFILING
+fi

You already tested for CONFIG_EXPERIMENTAL above..  Also you want
an (EXPERIMENTAL) mark.

Together with the above suggestion drivers/oprofile/Config.in should
look roughly like:

if [ "$CONFIG_EXPERIMENTAL" = "y" ]; then
   mainmenu_option next_comment
   comment 'Profiling support'
   bool 'Profiling support (EXPERIMENTAL)' CONFIG_PROFILING
   if [ "$CONFIG_PREEMPT" != "y" -a "$CONFIG_PROFILING" = "y" ]; then
      dep_tristate '  OProfile system profiling (EXPERIMENTAL)' CONFIG_OPROFILE
   fi
   endmenu
fi

OTOH I wonder whether you really want an submenu.  It could as well just
be part of the general settings one.

+obj-$(CONFIG_OPROFILE) += oprofile.o
+ 
+oprofile-objs := oprof.o cpu_buffer.o event_buffer.o \
+                       buffer_sync.o oprofilefs.o \
+                       oprofile_files.o
+ 
+include $(ARCH)/Makefile
+ 
+include $(TOPDIR)/Rules.make

Usually <foo>-objs is below obj-*, but that's just cosmetic.  The Makefile
inclusion seems very wrong to me.  Why do you need it?

Shouldn't drivers/oprofile/i386/ be arch/i386/oprofile??

/**
+ * \file oprofilefs.c
+ * Copyright 2002 the LyX Team
+ * Read the file COPYING
+ *
+ * \author John Levon <[e-mail]>
+ *
+ * Based on fs/binfmt_misc.c
+ *
+ * A simple filesystem for configuration and
+ * access of oprofile.
+ */

Why is this copyright different from the others?

ssize_t fail_write(struct file * file, const char * buf, size_t count, loff_t *
offset)
+{
+       return -EPERM;
+}

Why don't you just set no write method at all?

Maybe oprofilefs wants to become a generic version, it has nothing
oprofile-specific..

#ifndef CONFIG_PROFILING
+
+asmlinkage int sys_lookup_dcookie(unsigned long cookie, char * buf, size_t
len)+{
+       return -ENOSYS;
+}
+ 
+#else

Please use cond_syscall() and compile that file only for CONFIG_PROFILING set.

+       struct dcookie_struct * d_cookie; /* cookie, if any */

Make that conditional on CONFIG_PROFILING?

/* Profile hooks */
+#ifdef CONFIG_PROFILING
+EXPORT_SYMBOL(profile_event_register);
+EXPORT_SYMBOL(profile_event_unregister);
+EXPORT_SYMBOL(dcookie_register);
+EXPORT_SYMBOL(dcookie_unregister);
+EXPORT_SYMBOL(get_dcookie);
+#endif

Put that exports into profile.c?