Hi Roland,
Le 14.10.2013 20:19, Roland Dreier a écrit :
On Mon, Oct 14, 2013 at 8:46 AM, Yann Droneaud <[email protected]>
wrote:
Let's try for inclusion in v3.12-rc6
Hi everyone. First of sorry for being incommunicado for so long, been
swamped with a lot of stuff.
Anyway, given that everyone seems to agree we want something along the
lines of this series, but that the series is not ready yet, it really
seems to be that at this point the best option is Yann's simple patch
to temporarily disable these commands.
Can anyone talk me out of that?
From my point of view:
- commit 400dbc9 "IB/core: Infrastructure for extensible uverbs
commands"
was introduced with the purpose of providing ground for new commands,
but fail to actually provide real benefit, it just seems complicated
for nothing.
In fact, when reading the commit, I think the code is not doing
what's advertised in the commit message and in the extended command
header:
What's the point of splitting "core" from "provider" buffer ?
- commit 436f2ad "IB/core: Export ib_create/destroy_flow through uverbs"
rely on the extensible uverbs infrastructure introduced in commit
400dbc9, but it's not clear why it needs the infrastructure,
perhaps bigger command buffer is needed for large set of flow_spec ?
More important, ABI/API exposed would welcome some cleanups:
- The public data structures need a bit of rework: renaming, removing
struct ib_kern_spec
- The size of the flow_spec list is computed by subtracting the
command
header length and command to the total length.
kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
sizeof(struct ib_uverbs_cmd_hdr_ex);
The size must not depend on the command header, I believe they are
part
of different layer. cmd.flow_attr.size should be the size of the
flow_spec list.
- The parsing flow_spec is a bit incorrect, it goes wrong at
manipulating the size
of flow_spec list. Additionally it should not rely on underflow
kern_attr_size to detect error.
- commit 22878db "IB/core: Better checking of userspace values for
receive flow steering"
add checks for negative value on unsigned variables
- patch https://patchwork.kernel.org/patch/2924341/
"IB/core: clarify overflow/underflow checks on ib_create/destroy_flow"
is improving ib_uverbs_create_flow() and ib_uverbs_destroy_flow()
to fix the remarks I've made against commit 436f2ad and 22878db
but introduce a bug, not adding the size of flow_attr, when allocating
memory
for the flow_attr + flow_specs.
- Additionally, the improved extensible command scheme require changes
in ib_uverbs_create_flow() and ib_uverbs_destroy_flow() function
to use a new prototypes.
I've proposed fixe for ib_uverbs_create_flow(), ib_uverbs_destroy_flow()
and public API.
I've proposed an improvement of the extensible scheme based on my
understanding
of the intent described in the commit 400dbc9 message. This scheme seems
better,
but it's not the silver bullet: it doesn't address new requirements
provided by Matan Barak:
http://marc.info/[email protected]
> In addition, commands that have extensible sub-structures (for
example,
> extended address handle in QP attributes in IP based addressing
patch)
> should be given as a different UDATA in the cmd structure.
Therefore, we
> need a comp_mask in the cmd structure header to describe which UDATA
> structure are included.
http://marc.info/[email protected]
> Regarding the last patch, you are right that it simplifies things
for
> creating new uverbs where command parts are in-lined one after
another,
> but the infrastructure got a bit more complex.
> If we're going to this direction, I think the we should also deal
with
> the problem of extending one of the command parts. Currently, we'll
have
> to put a comp_mask in the in-lined command part, consume this
command
> part and then continue with the other parts. It might be better than
> using a pointer, but this put the burden of serializing the command
> buffer into the kernel structures onto the uverb command writer.
> We might want to avoid this.
> Furthermore, the comp_mask of the command is different than the
> comp_mask of the response. Therefore, I don't think we should pass
the
> command's comp_mask to the uverb as a pointer, but just pass a
pointer
> to value 0 that the uverb will set.
http://marc.info/[email protected]
> I think that's a good idea to make the comp_mask field a part of our
> infrastructure. But, I think we should consider making this
symmetrical.
> If we use the command's comp_mask as an input parameter, shouldn't
we
> use the response's comp_mask as an output parameter ?
> What's about the provider's comp_mask ?
http://marc.info/[email protected]
> This infrastructure (as well as the original one) doesn't deal with
the
> case we discussed about in the previous patches. When a command
contains
> a structure that can be extended, we don't have a clean nice way of
> doing that. Though, because it's a limitation of the original design
and
> this patch cleans up the functionality of the original
infrastructure,
> I'm in favor of merging it.
I would summarize:
- having the ability to extend multiple part of the command,
independently of the other;
- same for the response;
- having comp_mask in command for uverbs (eg. core/) and for provider
(eg. hw/);
- same for the response.
I've provided a patch to handle "comp_mask" in response,
but I don't feel confident in it: design doesn't sound clean.
While I would like to address the unlimited command expansion problem,
I think the scheme I've proposed is the wrong way. But in the same time
I don't believe I have a good way to do it.
The proposed new scheme is only good for core / hw split.
But, in the mean time, we failed at providing the required
fixes/cleanups before v3.12-rc5.
We have not agreed on a definitive, small, urgent patchset for
v3.12-rc5.
And I think, and it's easy to think so, that Linus will be very upset to
see more
than a couple of patches for the Infiniband subsystem for v3.12-rc6. If
there's a v3.12-rc6.
So instead of fighting to squeeze the last good patch hunk for v3.12, we
should think
on what command/response scheme we need for v3.13.
Regards.
--
Yann Droneaud
OPTEYA
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
