On Wed, 13 Feb 2008 10:32:20 -0600
Corey Minyard <[EMAIL PROTECTED]> wrote:
> From: Konstantin Baydarov <[EMAIL PROTECTED]>
>
> Atomics are a lot more efficient and neat than using a lock.
>
Yes, but...
> +struct ipmi_stats
> +{
> + /* Commands we got that were invalid. */
> + atomic_t sent_invalid_commands;
> +
> + /* Commands we sent to the MC. */
> + atomic_t sent_local_commands;
> + /* Responses from the MC that were delivered to a user. */
> + atomic_t handled_local_responses;
> + /* Responses from the MC that were not delivered to a user. */
> + atomic_t unhandled_local_responses;
> +
> + /* Commands we sent out to the IPMB bus. */
> + atomic_t sent_ipmb_commands;
> + /* Commands sent on the IPMB that had errors on the SEND CMD */
> + atomic_t sent_ipmb_command_errs;
> + /* Each retransmit increments this count. */
> + atomic_t retransmitted_ipmb_commands;
> + /* When a message times out (runs out of retransmits) this is
> + incremented. */
> + atomic_t timed_out_ipmb_commands;
> +
> + /* This is like above, but for broadcasts. Broadcasts are
> + *not* included in the above count (they are expected to
> + time out). */
> + atomic_t timed_out_ipmb_broadcasts;
> +
> + /* Responses I have sent to the IPMB bus. */
> + atomic_t sent_ipmb_responses;
> +
> + /* The response was delivered to the user. */
> + atomic_t handled_ipmb_responses;
> + /* The response had invalid data in it. */
> + atomic_t invalid_ipmb_responses;
> + /* The response didn't have anyone waiting for it. */
> + atomic_t unhandled_ipmb_responses;
> +
> + /* Commands we sent out to the IPMB bus. */
> + atomic_t sent_lan_commands;
> + /* Commands sent on the IPMB that had errors on the SEND CMD */
> + atomic_t sent_lan_command_errs;
> + /* Each retransmit increments this count. */
> + atomic_t retransmitted_lan_commands;
> + /* When a message times out (runs out of retransmits) this is
> + incremented. */
> + atomic_t timed_out_lan_commands;
> +
> + /* Responses I have sent to the IPMB bus. */
> + atomic_t sent_lan_responses;
> +
> + /* The response was delivered to the user. */
> + atomic_t handled_lan_responses;
> + /* The response had invalid data in it. */
> + atomic_t invalid_lan_responses;
> + /* The response didn't have anyone waiting for it. */
> + atomic_t unhandled_lan_responses;
> +
> + /* The command was delivered to the user. */
> + atomic_t handled_commands;
> + /* The command had invalid data in it. */
> + atomic_t invalid_commands;
> + /* The command didn't have anyone waiting for it. */
> + atomic_t unhandled_commands;
> +
> + /* Invalid data in an event. */
> + atomic_t invalid_events;
> + /* Events that were received with the proper format. */
> + atomic_t events;
> +};
The code forgot to initialise all of these.
It just so happens that the all-bits-zero pattern works correctly for all
current architectures, so the code should work OK. But there is no reason
(I hope) why an architecture cannot implement atomic_t as
struct atomic_t {
int counter;
spinlock_t lock;
};
in which case the results of ATOMIC_INIT() may _not_ be all-zeroes, in
which case the code will deadlock.
So. It works, but it's grubby. Do you still wish to proceed?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
