Re: [PATCH for-next 1/9] IB/core: Introduce peer client interface

[Yishai Hadas <yishaih-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 10/1/2014 7:34 PM, Bart Van Assche wrote:
> On 10/01/14 17:18, Yishai Hadas wrote:
>> +static int num_registered_peers;
>
> Is the only purpose of this variable to check whether or not 
> peer_memory_list is empty ? In that case please drop this variable and 
> use list_empty() instead.
     Agree.
>
>> +static int ib_invalidate_peer_memory(void *reg_handle, void 
>> *core_context)
>> +
>> +{
>> +    return -ENOSYS;
>> +}
> Please follow the Linux kernel coding style which means no empty line 
> above the function body.
     OK
>
>> +#define PEER_MEM_MANDATORY_FUNC(x) {\
>> +    offsetof(struct peer_memory_client, x), #x }
>
> Shouldn't the opening brace have been placed on the same line as the 
> offsetof() macro to improve readability ?
>
     OK
>> +    if (invalidate_callback) {
>> +        *invalidate_callback = ib_invalidate_peer_memory;
>> +        ib_peer_client->invalidation_required = 1;
>> +    }
>> +    mutex_lock(&peer_memory_mutex);
>> +    list_add_tail(&ib_peer_client->core_peer_list, &peer_memory_list);
>> +    num_registered_peers++;
>> +    mutex_unlock(&peer_memory_mutex);
>> +    return ib_peer_client;
>
> Please insert an empty line before mutex_lock() and after mutex_unlock().
>
     OK
>> +void ib_unregister_peer_memory_client(void *reg_handle)
>> +{
>> +    struct ib_peer_memory_client *ib_peer_client =
>> +        (struct ib_peer_memory_client *)reg_handle;
>
> No cast is needed when assigning a void pointer to a non-void pointer.
>
     Agree.
>> +struct peer_memory_client {
>> +    char    name[IB_PEER_MEMORY_NAME_MAX];
>> +    char    version[IB_PEER_MEMORY_VER_MAX];
>> +    /* The peer-direct controller (IB CORE) uses this callback to 
>> detect if a virtual address is under
>> +     * the responsibility of a specific peer direct client. If the 
>> answer is positive further calls
>> +     * for memory management will be directed to the callback of 
>> this peer driver.
>> +     * Any peer internal error should resulted in a zero answer, in 
>> case address range
>> +     * really belongs to the peer, no owner will be found and 
>> application will get an error
>> +     * from IB CORE as expected.
>> +     * Parameters:
>> +        addr                  [IN]  - virtual address to be checked 
>> whether belongs to.
>> +        size                  [IN]  - size of memory area starting 
>> at addr.
>> +        peer_mem_private_data [IN]  - The contents of ib_ucontext-> 
>> peer_mem_private_data.
>> +                          This parameter allows usage of the 
>> peer-direct
>> +                          API in implementations where it is impossible
>> +                          to detect if the memory belongs to the device
>> +                          based upon the virtual address alone. In such
>> +                          cases, the peer device can create a special
>> +                          ib_ucontext, which will be associated with 
>> the
>> +                          relevant peer memory.
>> +        peer_mem_name         [IN]  - The contents of ib_ucontext-> 
>> peer_mem_name.
>> +                          Used to identify the peer memory client that
>> +                          initialized the ib_ucontext.
>> +                          This parameter is normally used along with
>> +                          peer_mem_private_data.
>> +        client_context        [OUT] - peer opaque data which holds a 
>> peer context for
>> +                          the acquired address range, will be provided
>> +                          back to the peer memory in subsequent
>> +                          calls for that given memory.
>> +
>> +    * Return value:
>> +    *    1 - virtual address belongs to the peer device, otherwise 0
>> +    */
>> +    int (*acquire)(unsigned long addr, size_t size, void 
>> *peer_mem_private_data,
>> +               char *peer_mem_name, void **client_context);
>> +    /* The peer memory client is expected to pin the physical pages 
>> of the given address range
>> +     * and to fill sg_table with the information of the
>> +     * physical pages associated with the given address range. This 
>> function is
>> +     * equivalent to the kernel API of get_user_pages(), but targets 
>> peer memory.
>> +     * Parameters:
>> +        addr           [IN] - start virtual address of that given 
>> allocation.
>> +        size           [IN] - size of memory area starting at addr.
>> +        write          [IN] - indicates whether the pages will be 
>> written to by the caller.
>> +                      Same meaning as of kernel API get_user_pages, 
>> can be
>> +                      ignored if not relevant.
>> +        force          [IN] - indicates whether to force write 
>> access even if user
>> +                      mapping is readonly. Same meaning as of kernel 
>> API
>> +                      get_user_pages, can be ignored if not relevant.
>> +        sg_head        [IN/OUT] - pointer to head of struct sg_table.
>> +                      The peer client should allocate a table big
>> +                      enough to store all of the required entries. This
>> +                      function should fill the table with physical 
>> addresses
>> +                      and sizes of the memory segments composing this
>> +                      memory mapping.
>> +                      The table allocation can be done using 
>> sg_alloc_table.
>> +                      Filling in the physical memory addresses and 
>> size can
>> +                      be done using sg_set_page.
>> +        client_context [IN] - peer context for the given allocation, 
>> as received from
>> +                      the acquire call.
>> +        core_context   [IN] - opaque IB core context. If the peer 
>> client wishes to
>> +                      invalidate any of the pages pinned through 
>> this API,
>> +                      it must provide this context as an argument to 
>> the
>> +                      invalidate callback.
>> +
>> +    * Return value:
>> +    *    0 success, otherwise errno error code.
>> +    */
>> +    int (*get_pages)(unsigned long addr,
>> +             size_t size, int write, int force,
>> +             struct sg_table *sg_head,
>> +             void *client_context, void *core_context);
>> +    /* The peer-direct controller (IB CORE) calls this function to 
>> request from the
>> +     * peer driver to fill the sg_table with dma address mapping for 
>> the peer memory exposed.
>> +     * The parameters provided have the parameters for calling 
>> dma_map_sg.
>> +     * Parameters:
>> +        sg_head        [IN/OUT] - pointer to head of struct 
>> sg_table. The peer memory
>> +                      should fill the dma_address & dma_length for
>> +                      each scatter gather entry in the table.
>> +        client_context [IN] - peer context for the allocation mapped.
>> +        dma_device     [IN] - the RDMA capable device which requires 
>> access to the
>> +                      peer memory.
>> +        dmasync        [IN] - flush in-flight DMA when the memory 
>> region is written.
>> +                      Same meaning as with host memory mapping, can 
>> be ignored if not relevant.
>> +        nmap           [OUT] - number of mapped/set entries.
>> +
>> +    * Return value:
>> +    *        0 success, otherwise errno error code.
>> +    */
>> +    int (*dma_map)(struct sg_table *sg_head, void *client_context,
>> +               struct device *dma_device, int dmasync, int *nmap);
>> +    /* This callback is the opposite of the dma map API, it should 
>> take relevant actions
>> +     * to unmap the memory.
>> +    * Parameters:
>> +        sg_head        [IN/OUT] - pointer to head of struct 
>> sg_table. The peer memory
>> +                      should fill the dma_address & dma_length for
>> +                      each scatter gather entry in the table.
>> +        client_context [IN] - peer context for the allocation mapped.
>> +        dma_device     [IN] - the RDMA capable device which requires 
>> access to the
>> +                      peer memory.
>> +        dmasync        [IN] - flush in-flight DMA when the memory 
>> region is written.
>> +                      Same meaning as with host memory mapping, can 
>> be ignored if not relevant.
>> +        nmap           [OUT] - number of mapped/set entries.
>> +
>> +    * Return value:
>> +    *    0 success, otherwise errno error code.
>> +    */
>> +    int (*dma_unmap)(struct sg_table *sg_head, void *client_context,
>> +             struct device  *dma_device);
>> +    /* This callback is the opposite of the get_pages API, it should 
>> remove the pinning
>> +     * from the pages, it's the peer-direct equivalent of the kernel 
>> API put_page.
>> +     * Parameters:
>> +        sg_head        [IN] - pointer to head of struct sg_table.
>> +        client_context [IN] - peer context for that given allocation.
>> +    */
>> +    void (*put_pages)(struct sg_table *sg_head, void *client_context);
>> +    /* This callback returns page size for the given allocation
>> +     * Parameters:
>> +        sg_head        [IN] - pointer to head of struct sg_table.
>> +        client_context [IN] - peer context for that given allocation.
>> +    * Return value:
>> +    *    Page size in bytes
>> +    */
>> +    unsigned long (*get_page_size)(void *client_context);
>> +    /* This callback is the opposite of the acquire call, let peer 
>> release all resources associated
>> +     * with the acquired context. The call will be performed only 
>> for contexts that have been
>> +     * successfully acquired (i.e. acquire returned a non-zero value).
>> +     * Parameters:
>> +     *    client_context [IN] - peer context for the given allocation.
>> +    */
>> +    void (*release)(void *client_context);
>> +
>> +};
>
> All these comments inside a struct make a struct definition hard to 
> read. Please use kernel-doc style instead. See also 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt.
>
     Thanks, will fix in next series to match the kernel-doc style.
> Thanks,
>
> Bart.
>
> -- 
> 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

--
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