Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1563339 > unrolled thread

Re: [PATCH v2] jump_label: reduce the size of struct static_key

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] jump_label: reduce the size of struct static_key Ingo Molnar <mingo@kernel.org> - 2017-01-20 08:30 +0100
    Re: [PATCH v2] jump_label: reduce the size of struct static_key Jason Baron <jbaron@akamai.com> - 2017-01-20 21:10 +0100
      Re: [PATCH v2] jump_label: reduce the size of struct static_key Ingo Molnar <mingo@kernel.org> - 2017-01-21 08:10 +0100
        Re: [PATCH v2] jump_label: reduce the size of struct static_key Jason Baron <jbaron@akamai.com> - 2017-01-23 21:50 +0100

#1563339 — Re: [PATCH v2] jump_label: reduce the size of struct static_key

* Jason Baron <jbaron@akamai.com> wrote:

>  struct static_key {
>  	atomic_t enabled;
> +/*
> + * bit 0 => 1 if key is initially true
> + *	    0 if initially false
> + * bit 1 => 1 if points to struct static_key_mod
> + *	    0 if points to struct jump_entry
> + */
> +	union {
> +		unsigned long type;
> +		struct jump_entry *entries;
> +		struct static_key_mod *next;
> +	};


> +			key->type = (unsigned long)jlm2 | static_key_type(key);

> +		key->type = (unsigned long)jlm | static_key_type(key);

> +		*prev = (struct static_key_mod *)((unsigned long)jlm->next |
> +				((unsigned long)*prev & JUMP_TYPE_MASK));

> +			key->type = (unsigned long)jlm->entries |
> +					static_key_type(key);

I really hate these very ugly type conversions. Is there no cleaner way?

For example the last line could sure be written as:

			key->entries = jlm->entries;
			key->type |= static_key_type(key);

right?

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1563888

On 01/20/2017 02:19 AM, Ingo Molnar wrote:
>
> * Jason Baron <jbaron@akamai.com> wrote:
>
>>  struct static_key {
>>  	atomic_t enabled;
>> +/*
>> + * bit 0 => 1 if key is initially true
>> + *	    0 if initially false
>> + * bit 1 => 1 if points to struct static_key_mod
>> + *	    0 if points to struct jump_entry
>> + */
>> +	union {
>> +		unsigned long type;
>> +		struct jump_entry *entries;
>> +		struct static_key_mod *next;
>> +	};
>
>
>> +			key->type = (unsigned long)jlm2 | static_key_type(key);
>
>> +		key->type = (unsigned long)jlm | static_key_type(key);
>
>> +		*prev = (struct static_key_mod *)((unsigned long)jlm->next |
>> +				((unsigned long)*prev & JUMP_TYPE_MASK));
>
>> +			key->type = (unsigned long)jlm->entries |
>> +					static_key_type(key);
>
> I really hate these very ugly type conversions. Is there no cleaner way?
>
> For example the last line could sure be written as:
>
> 			key->entries = jlm->entries;
> 			key->type |= static_key_type(key);
>
> right?

Hi,

So that is going to over-write the static_key_type(key) in the first 
assignment. If the order is reversed we can't just |= in the pointer type.

How about:

static void jump_key_set_entries(struct static_key *key, struct 
jump_entry *entries)
{
        unsigned long type;

        type = static_key_type(key);
        key->entries = entries;
        key->type |= type;
}

and then we can also add:

void jump_key_set_mod(struct static_key *key, struct static_key_mod *mod)

doing basically the same thing. That will avoid the casts that you 
called out.

better?

Thanks,

-Jason

[toc] | [prev] | [next] | [standalone]

#1564065

* Jason Baron <jbaron@akamai.com> wrote:

> >For example the last line could sure be written as:
> >
> >			key->entries = jlm->entries;
> >			key->type |= static_key_type(key);
> >
> >right?
> 
> Hi,
> 
> So that is going to over-write the static_key_type(key) in the first
> assignment. If the order is reversed we can't just |= in the pointer type.

Indeed, I missed that.

> How about:
> 
> static void jump_key_set_entries(struct static_key *key, struct jump_entry *entries)
> {
>        unsigned long type;
> 
>        type = static_key_type(key);
>        key->entries = entries;
>        key->type |= type;
> }
> 
> and then we can also add:
> 
> void jump_key_set_mod(struct static_key *key, struct static_key_mod *mod)
> 
> doing basically the same thing. That will avoid the casts that you called
> out.
> 
> better?

Yeah - and it should generate the exact same code, right?

I'd also add a short comment to the helper function that points out the 
union/aliasing, in case anyone is wondering.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1565290

On 01/21/2017 02:07 AM, Ingo Molnar wrote:
>
> * Jason Baron <jbaron@akamai.com> wrote:
>
>>> For example the last line could sure be written as:
>>>
>>> 			key->entries = jlm->entries;
>>> 			key->type |= static_key_type(key);
>>>
>>> right?
>>
>> Hi,
>>
>> So that is going to over-write the static_key_type(key) in the first
>> assignment. If the order is reversed we can't just |= in the pointer type.
>
> Indeed, I missed that.
>
>> How about:
>>
>> static void jump_key_set_entries(struct static_key *key, struct jump_entry *entries)
>> {
>>        unsigned long type;
>>
>>        type = static_key_type(key);
>>        key->entries = entries;
>>        key->type |= type;
>> }
>>
>> and then we can also add:
>>
>> void jump_key_set_mod(struct static_key *key, struct static_key_mod *mod)
>>
>> doing basically the same thing. That will avoid the casts that you called
>> out.
>>
>> better?
>
> Yeah - and it should generate the exact same code, right?

yes, looks identical.

>
> I'd also add a short comment to the helper function that points out the
> union/aliasing, in case anyone is wondering.
>

Ok, I'll add that to v3.

Thanks,

-Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web