On Thu, Apr 07, 2016 at 09:02:43PM +0000, Daniel Jurgens wrote:
> On 4/7/2016 11:31 AM, Leon Romanovsky wrote:
> > On Thu, Apr 07, 2016 at 02:33:54AM +0300, Dan Jurgens wrote:
> >
> >> + if (sec->qp == sec->qp->real_qp) {
> >> + /* The caller of this function holds the QP security
> >> + * mutex so this list traversal is safe
> >> + */
> >
> > Did the comment below pass checkpatch.pl?
>
> It did.
>
> >> + list_for_each_entry(shared_qp_sec,
> >> + &sec->shared_qp_list,
> >> + shared_qp_list) {
> >
> > Is this list always needed to be protected by lock?
> > In general, I prefer to see lock/unlock operations near protected code.
> > Otherwise, it is hard to follow all lock/unlock paths.
>
> The mutex is required, and I'm not sure how to push it lower safely.
> Consider the situation where a QP is being modified concurrently with an
> open_shared_qp. Unless the lock is held the entire time for each
> operation open_shared_qp could check permission using a ib_qp_security
> struct in a partially modified state, i.e. there could be a mix of new
> and old settings for the port/pkey index/alternate path. Shared QPs use
> the port and pkey setting for the underlying real QP with the shared qp
> q_security.
Common flow is to do everything in one place:
1. lock
2. check permissions
3. change
4. release lock
Your flow is much broader.
1. lock
2. check permission
3. do work
4. change
5. do work
6. release lock.
Attachment:
signature.asc
Description: Digital signature
