Re: [PATCH v2 1/3] rte: add keep alive functionality

[Remy Horton <remy.horton@intel.com>]

'noon,

On 23/10/2015 15:27, Wiles, Keith wrote:
>> +	uint32_t __rte_cache_aligned state_flags[RTE_KEEPALIVE_MAXCORES];
> Normally I see the __rte_cache_aligned at the end of the line before
> the ‘;’ did you have a reason to have it here? If not then I would
> move it to the end to look the same as the others. I did a quick grop
> in the code and that is the normal location.
>
> My next question is why not align the whole, which would do the same
> thing. I did not check the compiler output, but I was thinking it
> would possible leave gaps in the structure for bytes we can not use
> normally, but maybe that is not a problem.

Each element of state_flags is assigned to a different LCore, so they 
have to be individually cache-aligned. The gaps it leaves behind are 
unavoidable.


> Next it appears the state_flags is only being set to 0-3, which means
> it does not need to be a uint43_t, but could be a uint8_t, correct?

Yes, but since it all needs to be cache aligned anyway, wouldn't 
actually gain anything.


>> +	keepcfg = malloc(sizeof(struct rte_keepalive));
>> +	if (keepcfg != NULL) {
>> +		for (idx_core = 0; idx_core < RTE_KEEPALIVE_MAXCORES; idx_core++) {
>> +			keepcfg->state_flags[idx_core] = 0;
>> +			keepcfg->active_cores[idx_core] = 0;
>> +		}
>
> Could you have done a calloc then you do not need the for loop to zero stuff?

Could do. It was written this way because the function originally took a 
structure rather than allocate one.


..Remy