On Sat, Oct 29, 2011 at 9:12 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Sat, 29 Oct 2011 23:44:53 +0400
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> 2011/10/29 Jeff Layton <jlayton@xxxxxxxxx>:
>> ...
>> >> + union {
>> >> + struct {
>> >> + atomic_t num_writes;
>> >> + atomic_t num_reads;
>> >> + atomic_t num_flushes;
>> >> + atomic_t num_oplock_brks;
>> >> + atomic_t num_opens;
>> >> + atomic_t num_closes;
>> >> + atomic_t num_deletes;
>> >> + atomic_t num_mkdirs;
>> >> + atomic_t num_posixopens;
>> >> + atomic_t num_posixmkdirs;
>> >> + atomic_t num_rmdirs;
>> >> + atomic_t num_renames;
>> >> + atomic_t num_t2renames;
>> >> + atomic_t num_ffirst;
>> >> + atomic_t num_fnext;
>> >> + atomic_t num_fclose;
>> >> + atomic_t num_hardlinks;
>> >> + atomic_t num_symlinks;
>> >> + atomic_t num_locks;
>> >> + atomic_t num_acl_get;
>> >> + atomic_t num_acl_set;
>> >> + } cifs_stats;
>> >> +#ifdef CONFIG_CIFS_SMB2
>> >> + struct {
>> >> + atomic_t smb2_com_sent[NUMBER_OF_SMB2_COMMANDS];
>> >> + atomic_t smb2_com_fail[NUMBER_OF_SMB2_COMMANDS];
>> >> + } smb2_stats;
>> >
>> > Is it really necessary to do this with atomics? Those can have
>> > significant performance impact (TLB flushes, and we don't seem to need the
>> > guarantees that they provide for simple counters like this. Perhaps
>> > these should be switched to per-cpu variables or just plain ints?
>>
>> I am not sure I understand your idea to make it as int. What's about
>> concurrency?
>>
>
> Sorry, I was reviewing these while on little sleep. We do need to worry
> about concurrency here so ints are probably not appropriate (unless
> these are already being incremented under a lock of some sort). Percpu
> variables though would be ideal for this.
Using per-cpu variables sounds reasonable, but that seems like something
that is lower priority (than much of your other patch feedback) for Pavel
to change in the short term since the current approach works
(albeit slightly slower when STATS2 is enabled).
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
