Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1358456 > unrolled thread

linux-next: manual merge of the rdma tree with the net-next tree

Back to article view | Back to linux.kernel

Contents

  linux-next: manual merge of the rdma tree with the net-next tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-03-16 02:00 +0100
    Re: linux-next: manual merge of the rdma tree with the net-next tree Maor Gottlieb <maor.linux@gmail.com> - 2016-03-16 15:30 +0100
    Re: linux-next: manual merge of the rdma tree with the net-next tree Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 18:20 +0100
      Re: linux-next: manual merge of the rdma tree with the net-next tree Doug Ledford <dledford@redhat.com> - 2016-03-16 18:40 +0100
        Re: linux-next: manual merge of the rdma tree with the net-next tree Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 18:50 +0100
      Re: linux-next: manual merge of the rdma tree with the net-next  tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-03-16 22:00 +0100
        Re: linux-next: manual merge of the rdma tree with the net-next tree Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-16 22:10 +0100
        Re: linux-next: manual merge of the rdma tree with the net-next tree Andrew Lunn <andrew@lunn.ch> - 2016-03-16 22:20 +0100
          Re: linux-next: manual merge of the rdma tree with the net-next  tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-03-16 23:40 +0100

#1358456 — linux-next: manual merge of the rdma tree with the net-next tree

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

[toc] | [next] | [standalone]

#1359042

2016-03-16 2:58 GMT+02:00 Stephen Rothwell <sfr@canb.auug.org.au>:
> 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

[toc] | [prev] | [next] | [standalone]

#1359181

On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <sfr@canb.auug.org.au> 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

[toc] | [prev] | [next] | [standalone]

#1359192

[Multipart message — attachments visible in raw view] — view raw

On 3/16/2016 1:18 PM, Linus Torvalds wrote:
> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <sfr@canb.auug.org.au> 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).

[toc] | [prev] | [next] | [standalone]

#1359200

On Wed, Mar 16, 2016 at 10:35 AM, Doug Ledford <dledford@redhat.com> wrote:
> On 3/16/2016 1:18 PM, Linus Torvalds wrote:
>> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <sfr@canb.auug.org.au> 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

[toc] | [prev] | [next] | [standalone]

#1359359 — Re: linux-next: manual merge of the rdma tree with the net-next tree

Hi Linus,

On Wed, 16 Mar 2016 10:18:33 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> On Tue, Mar 15, 2016 at 5:58 PM, Stephen Rothwell <sfr@canb.auug.org.au> 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

[toc] | [prev] | [next] | [standalone]

#1359365

On Wed, Mar 16, 2016 at 1:52 PM, Stephen Rothwell <sfr@canb.auug.org.au> 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

[toc] | [prev] | [next] | [standalone]

#1359369

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

[toc] | [prev] | [next] | [standalone]

#1359429 — Re: linux-next: manual merge of the rdma tree with the net-next tree

Hi Andrew,

On Wed, 16 Mar 2016 22:15:03 +0100 Andrew Lunn <andrew@lunn.ch> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web