Hello, Parav.
On Mon, Apr 04, 2016 at 07:22:38PM -0700, Parav Pandit wrote:
> > Is it actually customary to have rdma core module updated more
> > frequently separate from the kernel? Out-of-tree modules being
> > updated separately happens quite a bit but this is an in-kernel
> > module, which usually is tightly coupled with the rest of the kernel.
> >
> Yes.
> rdma core module is compiled as kernel module.
> Its often updated for new features, fixes.
> So kernel version can be one but RDMA core module(s) get updated more
> frequently than kernel.
As it is a fairly isolated domain, to certain extent, it could be okay
to let it go. At the same time, I know that these enterprise things
tend to go completely wayward and am worried about individual drivers
going crazy with custom attributes in a non-sensical way. The
interface this patch is proposing easily allows that and that at the
cost of internal engineering flexibility. I don't really want to be
caught up in a situation where we're forced to include broken usages
because that's what's already out in the wild. I personally would
much prefer the resources to be defined rigidly. Let's see how the
discussion with Christoph evolves.
> > I don't remember the details well but the code was vastly more complex
> > back then and the object lifetime management was muddy at best. If I
> > reviewed in a contradicting way, my apologies, but given the current
> > code, it'd be better to have objects creation upfront.
>
> Do you mean,
> try_charge() should
> lock()
> run loop to allocate in hierarchy, if not allocated.
> run loop again to charge.
> unlock()
>
> If so, I prefer to run the loop once.
In the original review message, I mentioned creating an interface
which creates the hierarchy of objects as necessary and returns the
target pool with lock held, can you please give it a shot? Something
like the following.
pool *get_pool(...)
{
lock;
if (target pool exists)
return pool w/ lock held;
create the pool hierarchically (might involve unlock);
if (succeeded)
return pool w/ lock held;
return NULL w/o lock;
}
> > It isn't necessarily about speed. It makes clear that the parent
> > always should exist and makes the code easier to read and update.
>
> It doesn't have to exist. It can get allocated when charging occurs.
> Otherwise even if rdma resources are not used, it ends up allocating
> rpool in hierarchy. (We talked this before)
Sure, create pools only for the used combinations but do that
hierarchically so that a child pool always has a parent. I can
promise you that the code will read a lot better with that.
> > I don't know why you end up missing basic patterns so badly. It's
> > making the review and iteration process pretty painful. Please don't
> > be confrontational and try to read the review comments assuming good
> > will.
> >
> My understanding of seq_printf() being blocking call and accessing
seq_printf() can be called from any context; otherwise, it would be a
horrible interface to use, wouldn't it?
> pool_info in spin lock context, made me allocate memory to get all
> values upfront through allocation.
> Now that the lock is going away, I can do what you have described above.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
