Hi,
I found a strange behaviour of checkpatch.pl.
The C statement:
return (type)value;
is regarded as return like a function form by checkpatch.pl.
So checkpatch.pl causes "Return is not a function." error
when processing statements like this.
I think statements like above are innocence. These are only doing type cast.
This patch removes the behaviour of checkpatch.pl.
But I don't have confidence about coding style of Linux kernel.
Is my thought correct? Or the behaviour of current checkpatch.pl is correct?
Request for comment.
Signed-off-by: Hitoshi Mitake <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bc4114f..04a876c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2117,7 +2117,7 @@ sub process {
}
# Return is not a function.
- if (defined($stat) && $stat =~ /^.\s*return(\s*)(\(.*);/s) {
+ if (defined($stat) && $stat =~ /^.\s*return(\s*)(\(.*)\);/s) {
my $spacing = $1;
my $value = $2;
--
1.5.6.5
On Tue, Nov 03, 2009 at 10:49:32AM +0900, Hitoshi Mitake wrote:
>
> Hi,
>
> I found a strange behaviour of checkpatch.pl.
>
> The C statement:
> return (type)value;
> is regarded as return like a function form by checkpatch.pl.
> So checkpatch.pl causes "Return is not a function." error
> when processing statements like this.
>
> I think statements like above are innocence. These are only doing type cast.
> This patch removes the behaviour of checkpatch.pl.
>
> But I don't have confidence about coding style of Linux kernel.
> Is my thought correct? Or the behaviour of current checkpatch.pl is correct?
> Request for comment.
Do you have a patch example which triggers this? If I test the current
version with this specific fragment I do not get a report. This one
collapses to '1value' which means it shouldn't trigger the report.
If you do have a specific patch which triggered this could you email it
over, also could you confirm the version of checkpatch which shows this
with checkpatch -v.
Thanks.
-apw
From: Andy Whitcroft <[email protected]>
Subject: Re: [PATCH][RFC] Removing wrong judgement of checkpatch.pl for return as function
Date: Fri, 6 Nov 2009 22:28:28 +0000
> On Tue, Nov 03, 2009 at 10:49:32AM +0900, Hitoshi Mitake wrote:
> >
> > Hi,
> >
> > I found a strange behaviour of checkpatch.pl.
> >
> > The C statement:
> > return (type)value;
> > is regarded as return like a function form by checkpatch.pl.
> > So checkpatch.pl causes "Return is not a function." error
> > when processing statements like this.
> >
> > I think statements like above are innocence. These are only doing type cast.
> > This patch removes the behaviour of checkpatch.pl.
> >
> > But I don't have confidence about coding style of Linux kernel.
> > Is my thought correct? Or the behaviour of current checkpatch.pl is correct?
> > Request for comment.
Thanks for your reply, Andy.
>
> Do you have a patch example which triggers this? If I test the current
> version with this specific fragment I do not get a report. This one
> collapses to '1value' which means it shouldn't trigger the report.
>
> If you do have a specific patch which triggered this could you email it
> over, also could you confirm the version of checkpatch which shows this
> with checkpatch -v.
>
Yes, I post the patch triggered the error I mentioned to lkml recently.
I'm attaching the patch the tail of this mail.
And the version of checkpatch.pl I used is 0.30.
% scripts/checkpatch.pl --version
Usage: checkpatch.pl [OPTION]... [FILE]...
Version: 0.30
Options:
...
--- below is the patch ---
This patch adds bench/sched-messaging.c.
This benchmark measures performance of scheduler and IPC mechanisms,
and is based on hackbench by Rusty Russell.
Example of usage:
% perf bench sched messaging -g 20 -l 1000 -s
5.432 # in sec
% perf bench sched messaging # run with default options
(20 sender and receiver processes per group)
(10 groups == 400 processes run)
Total time:0.308 sec
% perf bench sched messaging -t -g 20 # # be multi-thread, with 20 groups
(20 sender and receiver threads per group)
(20 groups == 800 threads run)
Total time:0.582 sec
Signed-off-by: Hitoshi Mitake <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Mike Galbraith <[email protected]>
---
tools/perf/bench/sched-messaging.c | 332 ++++++++++++++++++++++++++++++++++++
1 files changed, 332 insertions(+), 0 deletions(-)
create mode 100644 tools/perf/bench/sched-messaging.c
diff --git a/tools/perf/bench/sched-messaging.c b/tools/perf/bench/sched-messaging.c
new file mode 100644
index 0000000..36b62c5
--- /dev/null
+++ b/tools/perf/bench/sched-messaging.c
@@ -0,0 +1,332 @@
+/*
+ *
+ * builtin-bench-messaging.c
+ *
+ * messaging: Benchmark for scheduler and IPC mechanisms
+ *
+ * Based on hackbench by Rusty Russell <[email protected]>
+ * Ported to perf by Hitoshi Mitake <[email protected]>
+ *
+ */
+
+#include "../perf.h"
+#include "../util/util.h"
+#include "../util/parse-options.h"
+#include "../builtin.h"
+#include "bench.h"
+
+/* Test groups of 20 processes spraying to 20 receivers */
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <sys/time.h>
+#include <sys/poll.h>
+#include <limits.h>
+
+#define DATASIZE 100
+
+static int use_pipes = 0;
+static unsigned int loops = 100;
+static unsigned int thread_mode = 0;
+static unsigned int num_groups = 10;
+static int simple = 0;
+
+struct sender_context {
+ unsigned int num_fds;
+ int ready_out;
+ int wakefd;
+ int out_fds[0];
+};
+
+struct receiver_context {
+ unsigned int num_packets;
+ int in_fds[2];
+ int ready_out;
+ int wakefd;
+};
+
+static void barf(const char *msg)
+{
+ fprintf(stderr, "%s (error: %s)\n", msg, strerror(errno));
+ exit(1);
+}
+
+static void fdpair(int fds[2])
+{
+ if (use_pipes) {
+ if (pipe(fds) == 0)
+ return;
+ } else {
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0)
+ return;
+ }
+
+ barf(use_pipes ? "pipe()" : "socketpair()");
+}
+
+/* Block until we're ready to go */
+static void ready(int ready_out, int wakefd)
+{
+ char dummy;
+ struct pollfd pollfd = { .fd = wakefd, .events = POLLIN };
+
+ /* Tell them we're ready. */
+ if (write(ready_out, &dummy, 1) != 1)
+ barf("CLIENT: ready write");
+
+ /* Wait for "GO" signal */
+ if (poll(&pollfd, 1, -1) != 1)
+ barf("poll");
+}
+
+/* Sender sprays loops messages down each file descriptor */
+static void *sender(struct sender_context *ctx)
+{
+ char data[DATASIZE];
+ unsigned int i, j;
+
+ ready(ctx->ready_out, ctx->wakefd);
+
+ /* Now pump to every receiver. */
+ for (i = 0; i < loops; i++) {
+ for (j = 0; j < ctx->num_fds; j++) {
+ int ret, done = 0;
+
+again:
+ ret = write(ctx->out_fds[j], data + done,
+ sizeof(data)-done);
+ if (ret < 0)
+ barf("SENDER: write");
+ done += ret;
+ if (done < DATASIZE)
+ goto again;
+ }
+ }
+
+ return NULL;
+}
+
+
+/* One receiver per fd */
+static void *receiver(struct receiver_context* ctx)
+{
+ unsigned int i;
+
+ if (!thread_mode)
+ close(ctx->in_fds[1]);
+
+ /* Wait for start... */
+ ready(ctx->ready_out, ctx->wakefd);
+
+ /* Receive them all */
+ for (i = 0; i < ctx->num_packets; i++) {
+ char data[DATASIZE];
+ int ret, done = 0;
+
+again:
+ ret = read(ctx->in_fds[0], data + done, DATASIZE - done);
+ if (ret < 0)
+ barf("SERVER: read");
+ done += ret;
+ if (done < DATASIZE)
+ goto again;
+ }
+
+ return NULL;
+}
+
+static pthread_t create_worker(void *ctx, void *(*func)(void *))
+{
+ pthread_attr_t attr;
+ pthread_t childid;
+ int err;
+
+ if (!thread_mode) {
+ /* process mode */
+ /* Fork the receiver. */
+ switch (fork()) {
+ case -1:
+ barf("fork()");
+ break;
+ case 0:
+ (*func) (ctx);
+ exit(0);
+ break;
+ default:
+ break;
+ }
+
+ return (pthread_t)0;
+ }
+
+ if (pthread_attr_init(&attr) != 0)
+ barf("pthread_attr_init:");
+
+#ifndef __ia64__
+ if (pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN) != 0)
+ barf("pthread_attr_setstacksize");
+#endif
+
+ err = pthread_create(&childid, &attr, func, ctx);
+ if (err != 0) {
+ fprintf(stderr, "pthread_create failed: %s (%d)\n",
+ strerror(err), err);
+ exit(-1);
+ }
+ return childid;
+}
+
+static void reap_worker(pthread_t id)
+{
+ int proc_status;
+ void *thread_status;
+
+ if (!thread_mode) {
+ /* process mode */
+ wait(&proc_status);
+ if (!WIFEXITED(proc_status))
+ exit(1);
+ } else {
+ pthread_join(id, &thread_status);
+ }
+}
+
+/* One group of senders and receivers */
+static unsigned int group(pthread_t *pth,
+ unsigned int num_fds,
+ int ready_out,
+ int wakefd)
+{
+ unsigned int i;
+ struct sender_context *snd_ctx = malloc(sizeof(struct sender_context)
+ + num_fds * sizeof(int));
+
+ if (!snd_ctx)
+ barf("malloc()");
+
+ for (i = 0; i < num_fds; i++) {
+ int fds[2];
+ struct receiver_context *ctx = malloc(sizeof(*ctx));
+
+ if (!ctx)
+ barf("malloc()");
+
+
+ /* Create the pipe between client and server */
+ fdpair(fds);
+
+ ctx->num_packets = num_fds * loops;
+ ctx->in_fds[0] = fds[0];
+ ctx->in_fds[1] = fds[1];
+ ctx->ready_out = ready_out;
+ ctx->wakefd = wakefd;
+
+ pth[i] = create_worker(ctx, (void *)receiver);
+
+ snd_ctx->out_fds[i] = fds[1];
+ if (!thread_mode)
+ close(fds[0]);
+ }
+
+ /* Now we have all the fds, fork the senders */
+ for (i = 0; i < num_fds; i++) {
+ snd_ctx->ready_out = ready_out;
+ snd_ctx->wakefd = wakefd;
+ snd_ctx->num_fds = num_fds;
+
+ pth[num_fds+i] = create_worker(snd_ctx, (void *)sender);
+ }
+
+ /* Close the fds we have left */
+ if (!thread_mode)
+ for (i = 0; i < num_fds; i++)
+ close(snd_ctx->out_fds[i]);
+
+ /* Return number of children to reap */
+ return num_fds * 2;
+}
+
+static const struct option options[] = {
+ OPT_BOOLEAN('p', "pipe", &use_pipes,
+ "Use pipe() instead of socketpair()"),
+ OPT_BOOLEAN('t', "thread", &thread_mode,
+ "Be multi thread instead of multi process"),
+ OPT_INTEGER('g', "group", &num_groups,
+ "Specify number of groups"),
+ OPT_INTEGER('l', "loop", &loops,
+ "Specify number of loops"),
+ OPT_BOOLEAN('s', "simple-output", &simple,
+ "Do simple output (this maybe useful for"
+ "processing by scripts or graph tools like gnuplot)"),
+ OPT_END()
+};
+
+static const char * const bench_sched_message_usage[] = {
+ "perf bench sched messaging <options>",
+ NULL
+};
+
+int bench_sched_messaging(int argc, const char **argv,
+ const char *prefix __used)
+{
+ unsigned int i, total_children;
+ struct timeval start, stop, diff;
+ unsigned int num_fds = 20;
+ int readyfds[2], wakefds[2];
+ char dummy;
+ pthread_t *pth_tab;
+
+ argc = parse_options(argc, argv, options,
+ bench_sched_message_usage, 0);
+
+ pth_tab = malloc(num_fds * 2 * num_groups * sizeof(pthread_t));
+ if (!pth_tab)
+ barf("main:malloc()");
+
+ fdpair(readyfds);
+ fdpair(wakefds);
+
+ total_children = 0;
+ for (i = 0; i < num_groups; i++)
+ total_children += group(pth_tab+total_children, num_fds,
+ readyfds[1], wakefds[0]);
+
+ /* Wait for everyone to be ready */
+ for (i = 0; i < total_children; i++)
+ if (read(readyfds[0], &dummy, 1) != 1)
+ barf("Reading for readyfds");
+
+ gettimeofday(&start, NULL);
+
+ /* Kick them off */
+ if (write(wakefds[1], &dummy, 1) != 1)
+ barf("Writing to start them");
+
+ /* Reap them all */
+ for (i = 0; i < total_children; i++)
+ reap_worker(pth_tab[i]);
+
+ gettimeofday(&stop, NULL);
+
+ timersub(&stop, &start, &diff);
+
+ if (simple)
+ printf("%lu.%03lu\n", diff.tv_sec, diff.tv_usec/1000);
+ else {
+ printf("(%d sender and receiver %s per group)\n",
+ num_fds, thread_mode ? "threads" : "processes");
+ printf("(%d groups == %d %s run)\n\n",
+ num_groups, num_groups * 2 * num_fds,
+ thread_mode ? "threads" : "processes");
+ printf("\tTotal time:%lu.%03lu sec\n",
+ diff.tv_sec, diff.tv_usec/1000);
+ }
+
+ return 0;
+}
--
1.6.5.2
On Sat, Nov 07, 2009 at 10:14:29AM +0900, Hitoshi Mitake wrote:
> Yes, I post the patch triggered the error I mentioned to lkml recently.
> I'm attaching the patch the tail of this mail.
> + return (pthread_t)0;
Ahh, now I see how this could occur. My test case has a variable where
this has a 0. I think I've fixed this. Could you test with the version
at the URL below (it may take a little time to replicate):
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing
Thanks.
-apw
From: Andy Whitcroft <[email protected]>
Subject: Re: [PATCH][RFC] Removing wrong judgement of checkpatch.pl for return as function
Date: Thu, 12 Nov 2009 12:49:45 +0000
> On Sat, Nov 07, 2009 at 10:14:29AM +0900, Hitoshi Mitake wrote:
>
> > Yes, I post the patch triggered the error I mentioned to lkml recently.
> > I'm attaching the patch the tail of this mail.
>
> > + return (pthread_t)0;
>
> Ahh, now I see how this could occur. My test case has a variable where
> this has a 0. I think I've fixed this. Could you test with the version
> at the URL below (it may take a little time to replicate):
>
> http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing
I've tested your new checkpatch.pl with the patch I posted as a sample case.
And I didn't get an error I reported! It seems that checkpatch.pl works well.
Thanks,
Hitoshi