Re: [PATCH V2 2/2] mlx4/IB: Add support for enhanced atomic operations

[Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>]

 > | atomic_response = *va
 > | if (((cmp XOR *va) AND cmp_mask) is ZERO) then
 > |     *va = (*va AND NOT(swap_mask)) OR (swap AND swap_mask)
 > |
 > | return atomic_response

This is a great, terse description of the masked compare and swap
operation.  Do you think it would be more readable with C operations (ie
^ for XOR, & for AND, ~ for NOT, and | for OR)?  Also, I think it would
be good to use the exact field names as the work request field you add
(ie compare_add and compare_add_mask instead of cmp and cmp_mask).

(And as I said before, this belongs with the generic patch, not the mlx4
patch description)

 > Masked Fetch and Add (MFetchAdd)
 > The MFetchAdd Atomic operation extends the functionality of the standard IB
 > FetchAdd by allowing the user to split the target into multiple fields of
 > selectable length. The atomic add is done independently on each one of this
 > fields. A bit set in the field_boundary parameter specifies the field
 > boundaries. The pseudo code below describes the operation:

This is a bit harder to read, but I guess the operation is harder to
describe.  But what is really missing is the statement that the relevant
fields in the wr are compare_add and compare_add_mask.  Failing that,
it's really hard to know how to use this operation without looking at
the mlx4 source code.

 > +	props->device_cap_flags |= IB_DEVICE_MASKED_ATOMIC;

Is there no firmware dependency here?  If this really should be set
unconditionally, then please just roll it into the other flags that get
set for all devices.
-- 
Roland Dreier  <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html