Re: [PATCH/RFC 0/2] jump label: simplify API

[Steven Rostedt]

On Thu, 2010-12-16 at 14:22 -0500, Mathieu Desnoyers wrote:
> * Jason Baron (jbaron@xxxxxxxxxx) wrote:
> > Hi,
> > 
> > The first patch uses the storage space of the jump label key address
> > as a pointer into the update table. In this way, we can find all
> > the addresses that need to be updated without hashing.
> > 
> > The second patch introduces:
> > 
> > static __always_inline bool unlikely_switch(struct jump_label_key *key);
> > 
> > instead of the old JUMP_LABEL(key, label) macro.
> > 
> > In this way, jump labels become really easy to use:
> > 
> > Define:
> > 
> > 	struct jump_label_key jump_key;
> > 
> > Can be used as:
> > 
> > 	if (unlikely_switch(&jump_key))
> > 		do unlikely code
> 
> Ah, yes, that's an improvement!
> 
> I'm just wondering about the terminology here. Isn't that more a
> "branch" than a "switch" ?
> 
> I'm concerned about the fact that if we ever want to use the asm goto
> ability to jump to multiple targets (which is closer to a statically
> computed switch than a branch), we might want to reserve "switch" name
> for that rather than the branch.

Good point.

> 
> I wonder if the "if (unlikely_switch(&jump_key))" you propose above is
> the right thing to do. Why does the unlikely_ have to be included in the
> name ? Maybe there is a good reason for it, but it would be nice to have
> it spelled out. We might consider:
> 
> if (unlikely(static_branch(&jump_key)))
>   ...
> 
> instead.


Hmm, I see your point here too.


> For the switch statement, from the top of my head the idea would be to
> get something close to the following:
> 
> static __always_inline
> int static_switch_{3,4,5,6...}(struct jump_label_key *key);
> 
> e.g.:
> 
> static __always_inline
> int static_switch_3(struct jump_label_key *key)
> {
> 	asm goto("1:"
> 		JUMP_LABEL_INITIAL_NOP
> 		".pushsection __switch_table_3,  \"a\" \n\t"
> 		_ASM_PTR "%c0, 1b, %l[l_1], %l[l_2] \n\t"
> 		".popsection \n\t"
> 		: :  "i" (key) : : l_1, l_2 );
> 	return 0;
> l_1:
> 	return 1;
> l_2:
> 	return 2;
> }
> 
> switch(static_switch_3(&switch_key)) {
> 	case 0: .....
> 		break;
> 	case 1: .....
> 		break;
> 	case 2: .....
> 		break;
> }
> 
> (I have not tried to give that to gcc 4.5.x to see how the resulting
> assembly looks like. It would be interesting to see if it handles this
> case well)

Something to think about later (when we need such a thing). But I do
agree, perhaps calling it static_branch() instead, is a better idea.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/