Hi all,
Today's linux-next merge of the rdma tree got a conflict in:
drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
between commit:
60ab4584f5bf ("net/mlx5_core: Set flow steering dest only for forward rules")
from the net-next tree and commit:
b3638e1a7664 ("net/mlx5_core: Introduce forward to next priority action")
from the rdma tree.
I fixed it up (see below) and can carry the fix as necessary (no action
is required).
--
Cheers,
Stephen Rothwell
diff --cc drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
index e848d708d2b7,bf3446794bd5..000000000000
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
@@@ -73,10 -73,13 +73,13 @@@
#define BY_PASS_MIN_LEVEL (KENREL_MIN_LEVEL + MLX5_BY_PASS_NUM_PRIOS +\
LEFTOVERS_MAX_FT)
-#define KERNEL_MAX_FT 2
-#define KERNEL_NUM_PRIOS 1
+#define KERNEL_MAX_FT 3
+#define KERNEL_NUM_PRIOS 2
#define KENREL_MIN_LEVEL 2
+ #define ANCHOR_MAX_FT 1
+ #define ANCHOR_NUM_PRIOS 1
+ #define ANCHOR_MIN_LEVEL (BY_PASS_MIN_LEVEL + 1)
struct node_caps {
size_t arr_sz;
long *caps;
@@@ -360,8 -367,13 +367,13 @@@ static void del_rule(struct fs_node *no
memcpy(match_value, fte->val, sizeof(fte->val));
fs_get_obj(ft, fg->node.parent);
list_del(&rule->node.list);
+ if (rule->sw_action == MLX5_FLOW_CONTEXT_ACTION_FWD_NEXT_PRIO) {
+ mutex_lock(&rule->dest_attr.ft->lock);
+ list_del(&rule->next_ft);
+ mutex_unlock(&rule->dest_attr.ft->lock);
+ }
- fte->dests_size--;
- if (fte->dests_size) {
+ if ((fte->action & MLX5_FLOW_CONTEXT_ACTION_FWD_DEST) &&
+ --fte->dests_size) {
err = mlx5_cmd_update_fte(dev, ft,
fg->id, fte);
if (err)
@@@ -762,9 -835,9 +835,10 @@@ static struct mlx5_flow_rule *alloc_rul
if (!rule)
return NULL;
+ INIT_LIST_HEAD(&rule->next_ft);
rule->node.type = FS_TYPE_FLOW_DEST;
- memcpy(&rule->dest_attr, dest, sizeof(*dest));
+ if (dest)
+ memcpy(&rule->dest_attr, dest, sizeof(*dest));
return rule;
}
@@@ -783,12 -856,16 +857,17 @@@ static struct mlx5_flow_rule *add_rule_
return ERR_PTR(-ENOMEM);
fs_get_obj(ft, fg->node.parent);
- /* Add dest to dests list- added as first element after the head */
+ /* Add dest to dests list- we need flow tables to be in the
+ * end of the list for forward to next prio rules.
+ */
tree_init_node(&rule->node, 1, del_rule);
- list_add_tail(&rule->node.list, &fte->node.children);
+ if (dest && dest->type != MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE)
+ list_add(&rule->node.list, &fte->node.children);
+ else
+ list_add_tail(&rule->node.list, &fte->node.children);
- fte->dests_size++;
- if (fte->dests_size == 1)
+ if (dest)
+ fte->dests_size++;
+ if (fte->dests_size == 1 || !dest)
err = mlx5_cmd_create_fte(get_dev(&ft->node),
ft, fg->id, fte);
else
2016-03-16 2:58 GMT+02:00 Stephen Rothwell <[email protected]>:
> Hi all,
>
> Today's linux-next merge of the rdma tree got a conflict in:
>
> drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
>
> between commit:
>
> 60ab4584f5bf ("net/mlx5_core: Set flow steering dest only for forward rules")
>
> from the net-next tree and commit:
>
> b3638e1a7664 ("net/mlx5_core: Introduce forward to next priority action")
>
> from the rdma tree.
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
>
> --
> Cheers,
> Stephen Rothwell
>
> diff --cc drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> index e848d708d2b7,bf3446794bd5..000000000000
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> @@@ -73,10 -73,13 +73,13 @@@
> #define BY_PASS_MIN_LEVEL (KENREL_MIN_LEVEL + MLX5_BY_PASS_NUM_PRIOS +\
> LEFTOVERS_MAX_FT)
>
> -#define KERNEL_MAX_FT 2
> -#define KERNEL_NUM_PRIOS 1
> +#define KERNEL_MAX_FT 3
> +#define KERNEL_NUM_PRIOS 2
> #define KENREL_MIN_LEVEL 2
>
> + #define ANCHOR_MAX_FT 1
> + #define ANCHOR_NUM_PRIOS 1
> + #define ANCHOR_MIN_LEVEL (BY_PASS_MIN_LEVEL + 1)
> struct node_caps {
> size_t arr_sz;
> long *caps;
> @@@ -360,8 -367,13 +367,13 @@@ static void del_rule(struct fs_node *no
> memcpy(match_value, fte->val, sizeof(fte->val));
> fs_get_obj(ft, fg->node.parent);
> list_del(&rule->node.list);
> + if (rule->sw_action == MLX5_FLOW_CONTEXT_ACTION_FWD_NEXT_PRIO) {
> + mutex_lock(&rule->dest_attr.ft->lock);
> + list_del(&rule->next_ft);
> + mutex_unlock(&rule->dest_attr.ft->lock);
> + }
> - fte->dests_size--;
> - if (fte->dests_size) {
> + if ((fte->action & MLX5_FLOW_CONTEXT_ACTION_FWD_DEST) &&
> + --fte->dests_size) {
> err = mlx5_cmd_update_fte(dev, ft,
> fg->id, fte);
> if (err)
> @@@ -762,9 -835,9 +835,10 @@@ static struct mlx5_flow_rule *alloc_rul
> if (!rule)
> return NULL;
>
> + INIT_LIST_HEAD(&rule->next_ft);
> rule->node.type = FS_TYPE_FLOW_DEST;
> - memcpy(&rule->dest_attr, dest, sizeof(*dest));
> + if (dest)
> + memcpy(&rule->dest_attr, dest, sizeof(*dest));
>
> return rule;
> }
> @@@ -783,12 -856,16 +857,17 @@@ static struct mlx5_flow_rule *add_rule_
> return ERR_PTR(-ENOMEM);
>
> fs_get_obj(ft, fg->node.parent);
> - /* Add dest to dests list- added as first element after the head */
> + /* Add dest to dests list- we need flow tables to be in the
> + * end of the list for forward to next prio rules.
> + */
> tree_init_node(&rule->node, 1, del_rule);
> - list_add_tail(&rule->node.list, &fte->node.children);
> + if (dest && dest->type != MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE)
> + list_add(&rule->node.list, &fte->node.children);
> + else
> + list_add_tail(&rule->node.list, &fte->node.children);
> - fte->dests_size++;
> - if (fte->dests_size == 1)
> + if (dest)
> + fte->dests_size++;
> + if (fte->dests_size == 1 || !dest)
> err = mlx5_cmd_create_fte(get_dev(&ft->node),
> ft, fg->id, fte);
> else
Hi Stephen,
I reveiwed your merge and it's fine.
Thanks,
Maor
On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <[email protected]> wrote:
>
> I fixed it up (see below) and can carry the fix as necessary (no action
> is required).
Side note: can you change this wording for your manual merge script?
Last merge window (or was it the one before it?) we had confusion with
people who thought that "no action is required" means "you can just
ignore this entirely".
I want people who have known merge issues to at the very least
*mention* them to me when they send the pull request, and I also think
that trees that have merge conflicts that aren't just totally trivial
should also make sure that they have communicated with each other
about why the problem happened.
This is *particularly* true for the complete effing disaster that is
mellanox and rdma-vs-networking.
So please don't say "no action is required". Please make it clear that
there may not be any further action needed for linux-next itself, but
that other action may certainly be required.
Because I'm very close to not taking any rdma changes that touch
networking any more. Ever.
The Mellanox people are on my shit-list until they show that they can
actually act like responsible people and not just monkeys throwing
shit at the walls.
"No action required" is simply not true for Mellanox.
Linus
On 3/16/2016 1:18 PM, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <[email protected]> wrote:
>>
>> I fixed it up (see below) and can carry the fix as necessary (no action
>> is required).
>
> Side note: can you change this wording for your manual merge script?
> Last merge window (or was it the one before it?) we had confusion with
> people who thought that "no action is required" means "you can just
> ignore this entirely".
I certainly didn't take it that way regardless of the wording. I'm
keenly aware of the short leash you have Mellanox (and by extension
myself) on. I reviewed the merge in detail, enough to satisfy myself
that it was easy, correct, and that the code itself made the merge
obvious (such as the comment that flow table entries need to be last in
the lists in order to support priority transitions, which fairly handily
explained what needed to happen in the merge).
On Wed, Mar 16, 2016 at 10:35 AM, Doug Ledford <[email protected]> wrote:
> On 3/16/2016 1:18 PM, Linus Torvalds wrote:
>> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <[email protected]> wrote:
>>>
>>> I fixed it up (see below) and can carry the fix as necessary (no action
>>> is required).
>>
>> Side note: can you change this wording for your manual merge script?
>> Last merge window (or was it the one before it?) we had confusion with
>> people who thought that "no action is required" means "you can just
>> ignore this entirely".
>
> I certainly didn't take it that way regardless of the wording.
It was Or Gerlitz. You were cc'd, since it was the whole rdma Mellanox
mess. I quote from that thread:
"> However, the fact that it got resolved in linux-next is purely
> informational. It doesn't "fix" the conflict - it just means that both
> sides should have gotten informed about it. That doesn't mean that the
> conflict goes away or becomes better.
That's news to me. When such things happen and caught by Stephen, we
are getting an email saying something like
"Today's linux-next merge of the infiniband tree got a conflict
between commit X from net-next tree and commit Y from the infiniband
tree. I fixed it up (see below) and can carry the fix as necessary (no
action is required)."
Also asked around a bit and got to learn on Stephen using git rerere,
so all (no action needed note + seeing git rerere in action...) that
leaded me to think that indeed no action is required from our side,
but after reading your email (twice, so far), I realized that this was
wrong conclusion."
So that whole "no action is required" wording very much has caused
confusion before in the rdma camp.
Let's fix the wording. I'm indeed hopeful that the rdma camp is now
keenly aware of the issues, but that doesn't change the fact that the
wording has been problematic.
Linus
Hi Linus,
On Wed, 16 Mar 2016 10:18:33 -0700 Linus Torvalds <[email protected]> wrote:
>
> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <[email protected]> wrote:
> >
> > I fixed it up (see below) and can carry the fix as necessary (no action
> > is required).
>
> Side note: can you change this wording for your manual merge script?
> Last merge window (or was it the one before it?) we had confusion with
> people who thought that "no action is required" means "you can just
> ignore this entirely".
>
> I want people who have known merge issues to at the very least
> *mention* them to me when they send the pull request, and I also think
> that trees that have merge conflicts that aren't just totally trivial
> should also make sure that they have communicated with each other
> about why the problem happened.
>
> This is *particularly* true for the complete effing disaster that is
> mellanox and rdma-vs-networking.
>
> So please don't say "no action is required". Please make it clear that
> there may not be any further action needed for linux-next itself, but
> that other action may certainly be required.
Yeah, I can see your point. The "no action required" was a reaction to
people going off and rebasing their tree or dropping patches at any
sign of a conflict at all.
How about "This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may want also want to
consider cooperate with the maintainer of the conflicting tree to
minimise any particularly complex conflicts."
--
Cheers,
Stephen Rothwell
On Wed, Mar 16, 2016 at 1:52 PM, Stephen Rothwell <[email protected]> wrote:
>
> How about "This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may want also want to
> consider cooperate with the maintainer of the conflicting tree to
> minimise any particularly complex conflicts."
Yup, sounds fine.
Maybe you could even say "don't merge this to hide the problem",
because that has been another reaction in the past, but the above
already sounds pretty good.
Linus
Hi Stephen
> How about "This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may want also want to
Only the second want is required.
> consider cooperate with the maintainer of the conflicting tree to
cooperating
Andrew
Hi Andrew,
On Wed, 16 Mar 2016 22:15:03 +0100 Andrew Lunn <[email protected]> wrote:
>
> > How about "This is now fixed as far as linux-next is concerned, but any
> > non trivial conflicts should be mentioned to your upstream maintainer
> > when your tree is submitted for merging. You may want also want to
>
> Only the second want is required.
>
> > consider cooperate with the maintainer of the conflicting tree to
>
> cooperating
Thanks. Breakfast is not the best time to compose prose :-)
--
Cheers,
Stephen Rothwell
On Wed, Mar 16, 2016 at 7:44 PM, Linus Torvalds
<[email protected]> wrote:
> On Wed, Mar 16, 2016 at 10:35 AM, Doug Ledford <[email protected]> wrote:
>> On 3/16/2016 1:18 PM, Linus Torvalds wrote:
>>> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <[email protected]> wrote:
>>>> I fixed it up (see below) and can carry the fix as necessary (no action
>>>> is required).
>>> Side note: can you change this wording for your manual merge script?
>>> Last merge window (or was it the one before it?) we had confusion with
>>> people who thought that "no action is required" means "you can just
>>> ignore this entirely".
>> I certainly didn't take it that way regardless of the wording.
> It was Or Gerlitz. You were cc'd, since it was the whole rdma Mellanox
> mess. I quote from that thread:
>
> "> However, the fact that it got resolved in linux-next is purely
> > informational. It doesn't "fix" the conflict - it just means that both
> > sides should have gotten informed about it. That doesn't mean that the
> > conflict goes away or becomes better.
>
> That's news to me. When such things happen and caught by Stephen, we
> are getting an email saying something like
>
> "Today's linux-next merge of the infiniband tree got a conflict
> between commit X from net-next tree and commit Y from the infiniband
> tree. I fixed it up (see below) and can carry the fix as necessary (no
> action is required)."
>
> Also asked around a bit and got to learn on Stephen using git rerere,
> so all (no action needed note + seeing git rerere in action...) that
> leaded me to think that indeed no action is required from our side,
> but after reading your email (twice, so far), I realized that this was
> wrong conclusion."
> So that whole "no action is required" wording very much has caused
> confusion before in the rdma camp.
> Let's fix the wording. I'm indeed hopeful that the rdma camp is now
> keenly aware of the issues, but that doesn't change the fact that the
> wording has been problematic.
>> "[...] The Mellanox people are on my xxit-list until they show that they can
>> actually act like responsible people [...]"
Linus,
As I wrote you in that other thread you were quoting from there,
following to the happenings mentioned there, we took responsibility
and made bunch of corrective actions within Mellanox which got us to a
point where there was only one rdma/netdev-next conflict for the 4.6
merge window.
I know there's history here, and in the 4.5 cycle things were much
worse, but I still wanted to put things in their more precise place,
if you don't mind.
Or.
On Wed, Mar 23, 2016 at 4:04 PM, Or Gerlitz <[email protected]> wrote:
>
> I know there's history here, and in the 4.5 cycle things were much
> worse, but I still wanted to put things in their more precise place,
> if you don't mind.
We'll see how things shape up in the future. Once bitten, twice shy,
as they say.
Please make sure that Mellanox will not be a pain going forward, and
everything will be forgiven/forgotten.
Linus