On Wed, Jan 10, 2018 at 11:04 PM, Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
> On Wed, Jan 10, 2018 at 11:16:20AM +0530, Devesh Sharma wrote:
>
>> > +/*
>> > + * Allocate and initialize a context structure. This is called to create the
>> > + * driver wrapper, and context_offset is the number of bytes into the wrapper
>> > + * structure where the verbs_context starts.
>> > + */
>> > +void *_verbs_init_and_alloc_context(struct ibv_device *device, int cmd_fd,
>> > + size_t alloc_size,
>> > + struct verbs_context *context_offset)
>> > +{
>> > + void *drv_context;
>> > + struct verbs_context *context;
>> > +
>> > + drv_context = calloc(1, alloc_size);
>> > + if (!drv_context) {
>> > + errno = ENOMEM;
>> > + close(cmd_fd);
>> > + return NULL;
>> > + }
>> > +
>> > + context = (struct verbs_context *)((uint8_t *)drv_context +
>> > + (uintptr_t)context_offset);
>>
>> A wrapper macro would do better here?
>
> What would we call it? It is kinda of a reverse container of
may be get_ibverbs_context()
>
> Thing is, this is the only place that does this calculation and it is
> intimately tied to the definition of the
> verbs_init_and_alloc_context() macro, so it really is unique and
> special to this function.
If that is the case I don't mind leaving this as it is, may be it
would look better from code readability point of view if we wrap it.
>
> To elaborate on what is happening here..
>
> The driver calls
>
> cntx = verbs_init_and_alloc_context(vdev, cmd_fd, cntx, ibvctx);
>
> Where:
> struct bnxt_re_context *cntx;
>
> And the name 'ibvctx' is like container_of, where it refers to the
> struct member inside cntx:
>
> struct bnxt_re_context {
> struct verbs_context ibvctx;
>
> This allows the allocation function to both return the 'struct
> bnxt_re_context' and find the 'struct verbs_context' where the driver
> placed it.
>
> The alternative here is to force the driver to put the verbs_context
> at the start of the struct, then eliminate the context_offset entirely
> and replace it with a static assert scheme.
>
> Do you think that is nicer?
If there are no specific reasons to allow drivers to put this struct
anywhere in their defs,
then things could be simpler verb_context is placed in the beginning.
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
