public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Sasha Levin <levinsasha928@gmail.com>
Cc: David Laight <David.Laight@ACULAB.COM>,
	Steven Rostedt <rostedt@goodmis.org>,
	torvalds@linux-foundation.org, tj@kernel.org,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	ebiederm@xmission.com, neilb@suse.de, bfields@fieldses.org,
	ejt@redhat.com, snitzer@redhat.com, edumazet@google.com,
	josh@joshtriplett.org, rmallon@gmail.com, palves@redhat.com
Subject: Re: [PATCH v6] hashtable: introduce a small and naive hashtable
Date: Thu, 27 Sep 2012 09:11:18 -0400	[thread overview]
Message-ID: <20120927131118.GC30139@Krystal> (raw)
In-Reply-To: <50640F4E.4020305@gmail.com>

* Sasha Levin (levinsasha928@gmail.com) wrote:
> On 09/27/2012 10:25 AM, David Laight wrote:
> >>>> And even then, if we would do:
> >>>>
> >>>>  	for (i = 0; i < HASH_SIZE(hashtable); i++)
> >>>>  		if (!hlist_empty(&hashtable[i]))
> >>>>  			break;
> >>>>
> >>>>  	return i >= HASH_SIZE(hashtable);
> >>>>
> >>>> What happens if the last entry of the table is non-empty ?
> >>>
> >>> It still works, as 'i' is not incremented due to the break. And i will
> >>> still be less than HASH_SIZE(hashtable). Did you have *your* cup of
> >>> coffee today? ;-)
> >>
> >> Ahh, right! Actually I had it already ;-)
> > 
> > I tend to dislike the repeated test, gcc might be able to optimise
> > it away, but the code is cleaner written as:
> > 
> > 	for (i = 0; i < HASH_SIZE(hashtable); i++)
> > 		if (!hlist_empty(&hashtable[i]))
> > 			return false;
> > 	return true;
> 
> Right, the flag thing in the macro was there just to make it work
> properly as a macro.
> 
> >> Agreed that the flags should be removed. Moving to define + static
> >> inline is still important though.
> > 
> > Not sure I'd bother making the function inline.
> 
> I usually never make anything 'inline', I just let gcc do it's own
> thing when it compiles the code. If there are any objections
> please let me know before I send the new version.

AFAIK, gcc nowadays use "inline" only as a hint, because programmers
were using it everywhere, even where it should not have been used. This
is where the attribute always_inline becomes useful, if you really,
really want to inline. However, for kernel coding style consistency, it
might be better to use "static inline", because it is used everywhere
else in kernel headers. Or maybe are there some headers that do not use
"inline" I am not aware of ?

Moreover, if your thinking is that we do not need a static inline
function replicated at every caller, maybe we should introduce a
lib/hashtable.c that implements those 2 functions.

Thanks,

Mathieu

> 
> 
> Thanks,
> Sasha
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

  parent reply	other threads:[~2012-09-27 13:11 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-26 12:48 [PATCH v6] hashtable: introduce a small and naive hashtable Sasha Levin
2012-09-26 13:45 ` David Laight
2012-09-26 13:59   ` Steven Rostedt
2012-09-26 14:26     ` Sasha Levin
2012-09-26 14:39       ` Mathieu Desnoyers
2012-09-26 16:09         ` Steven Rostedt
2012-09-26 16:19           ` Mathieu Desnoyers
2012-09-27  8:25             ` David Laight
2012-09-27  8:33               ` Sasha Levin
2012-09-27 12:09                 ` Steven Rostedt
2012-09-27 13:11                 ` Mathieu Desnoyers [this message]
2012-09-27 13:30                   ` Steven Rostedt
2012-09-27 14:36                   ` David Laight
2012-09-27 13:03               ` Mathieu Desnoyers
2012-09-26 14:31   ` Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120927131118.GC30139@Krystal \
    --to=mathieu.desnoyers@efficios.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=akpm@linux-foundation.org \
    --cc=bfields@fieldses.org \
    --cc=ebiederm@xmission.com \
    --cc=edumazet@google.com \
    --cc=ejt@redhat.com \
    --cc=josh@joshtriplett.org \
    --cc=levinsasha928@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=palves@redhat.com \
    --cc=rmallon@gmail.com \
    --cc=rostedt@goodmis.org \
    --cc=snitzer@redhat.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox