Hi Roland, > -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > owner@xxxxxxxxxxxxxxx] On Behalf Of Roland Dreier > Sent: Tuesday, October 14, 2014 11:30 AM > Cc: Haggai Eran; linux-rdma@xxxxxxxxxxxxxxx; Shachar Raindel; Sagi > Grimberg > Subject: Re: [PATCH v1 for-next 06/16] IB/core: Implement support for > MMU notifiers regarding on demand paging regions > > > + notifiers_seq = atomic_read(&item->odp_data->notifiers_seq) + 1; > > + atomic_set(&item->odp_data->notifiers_seq, > > + notifiers_seq); > > Is this code really as silly as it looks, or is there some deep reason > for avoiding atomic_inc() that I'm missing? Do you need atomic_inc(), > since as far as I can tell all modification of notifiers_seq and > notifiers_count happens while holding the umem_mutex? > > In general I find it very suspicious that you have these two atomic_t > members, notifiers_seq and notifiers_count, but you never use anything > except atomic_read() and atomic_set() to access them. Why are they > atomic_t at all? > The sequence counter is read while not holding the lock. To avoid cases where the value is not completely updated due to an increase or decrease operation, resulting in a completely irrelevant value (can happen in some weird CPU architectures). We preferred to use an explicit atomic_t type, which guarantee atomic updates for such cases. We avoided atomic_inc as it results in a locked operation in intel platform, which is not needed if the operation is done under a lock. We therefore do an explicit read-modify-write, avoiding any locked operations. We will reexamine the code to determine if we can safely switch to normal variables. Thanks, --Shachar ��.n��������+%������w��{.n�����{���fk��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
