[ofiwg] thoughts on initializing libfabric structs

[Jason Gunthorpe]

On Mon, Jul 28, 2014 at 02:45:12PM -0400, Robert D. Russell wrote:

> >Having the library header initialize the structure is a legitimate
> >approach to ABI compatability, but don't use a bitmask, just use an
> >ABI version field, and carefully define a null value state for all
> >fields. That will avoid confusion.
> 
> I'm missing something here, because the static initializer should
> "carefully define a null value state for all fields".
> The whole idea of these initializers is exactly to ensure that all
> allocated fields are in fact initialized, whether or not
> the code that allocates the structure knows about or uses the fields.

The whole point of the bitmask is to provide a 'test for presence'
that does not require a defined 'null/not-present' value.

If you are willing/able to provide that via a defined
'null/not-present' value then don't use a bitmask.

A null value is not always easy to define, '0' is often a desirable
value for integers and boolean flags, for instance.

> >>static inline void
> >>fi_mr_attr_destroy(struct fi_mr_attr *attr)
> >>{
> >>	/* This can be a no-op for simple structures */
> >
> >Wow, that's a pain. There are many places where simple stack allocated
> >structures are sufficient and adding a API requirement to destroy them on
> >unwind is a huge burden, especially if the destroy is just a
> >NOP.
> 
> For simple stack allocated structures, the FI_FOO_INITIALIZER
> would be used, in which case neither
> fi_foo_init() nor fi_foo_destroy() are called.

That is not consistent with other users of this idiom (eg pthreads).

If there is a _destroy() then the _destroy *MUST* be called prior to
deallocating the memory.

The static initializer vs _init() is just a user choice.

> However, won't most compilers just optimize out a call to an empty
> static inline anyway?? gcc does.

Sure, but that isn't the point, you've converted something like:

struct initer foo = {};
 if (fabric_do(&foo) == FAILS)
     return FAILS;

Into this:

 struct initer foo = INITER_INIT;
 if (fabric_do(&foo) == FAILS)
 {
     initer_destroy(&foo);
     return FAILS;
 }
 initer_destroy(&foo);

Or maybe this
 struct initer foo = INITER_INIT;
 rc = fabric_do(&foo)
 initer_destroy(&foo);
 if (rc == FAILS)
     return FAILS;

Neither of which are particularly friendly or idiomatic.

And now there are many possibilities for interesting typos/thinkos.

> for which the provider was compiled.  Isn't the only way
> to know the allocation size of those earlier versions would be
> to #define FI_FOO_VERSION_2_SIZE 128  (for example), but
> the 128 would have to be supplied "by hand"?

In practice, you'd want to exactly preserves each structure as it was
at each release:

 struct fabric_foo_rel1 {...};
 struct fabric_foo_rel2 {...};
 struct fabric_foo_rel3 {...};
 typedef fabric_foo_rel3 fabric_foo;

This not only documents the exact structure in use at each ABI point,
but gives you something to sizeof() and something to verify offsetof()
against.

You can't really #define the size, since the required padding is
depending on the target system ABI.

> Wouldn't the name of the bit in the bit-mask (number 3) be a better
> target of the #ifdef, since that would not be defined at compile-time
> if that field were not present?  This would require the bit names be
> define constants, (as well as enum values if that is also desirable).

There are trade offs, features often introduce multiple fields at
once, so do you check each field individually when using bitfiles?

Ultimately, mistakes at this level are not serious as the compiler
automatically checks everything. Refering to a field name that does
not exist will not compile, not mis-operate.

So I wouldn't over complicate things. Even using autoconf to detect
the feature is not unreasonable.

Jason