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

Groups > linux.kernel > #1423472 > unrolled thread

Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link  goes down. "Jiang, Dave" <dave.jiang@intel.com> - 2016-06-15 23:40 +0200
    Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link  goes down. Logan Gunthorpe <logang@deltatee.com> - 2016-06-16 00:30 +0200
      Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link  goes down. "Jiang, Dave" <dave.jiang@intel.com> - 2016-06-16 00:30 +0200
        Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link  goes down. Logan Gunthorpe <logang@deltatee.com> - 2016-06-16 01:20 +0200

#1423472 — Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
> When the link goes down, the link_is_up flag did not return to
> false. This could have caused some subtle corner case bugs
> when the link goes up and down quickly.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Dave Jiang <dave.jiang@intel.com>

And all the other ntb_perf patches since there were no additional
changes. 

> ---
>  drivers/ntb/test/ntb_perf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index f0784e5..ae9d1b2 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct work_struct
> *work)
>  
>  	if (!perf->link_is_up)
>  		cancel_delayed_work_sync(&perf->link_work);
> +
> +	perf->link_is_up = false;
>  }
>  
>  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
> -- 
> 2.1.4
> 

[toc] | [next] | [standalone]

#1423550

Hey,

Actually, I have to retract this patch. After some more thorough testing
I'm finding an issue:

When you remove and re-install the ntb_perf module very quickly,
ntb_perf will occasionally miss the link up event. This is because the
link_cleanup work gets delayed long enough that it gets scheduled after
the link up event gets sent. It then cancels the link work that should
have occurred. Without this patch, it never happens because link_is_up
never returns to false.

I think the correct solution is to just remove the link_cleanup work and
do those actions immediately on receipt of the event. If there's
agreement on this I can re-spin it again.

Thanks,

Logan


On 15/06/16 03:33 PM, Jiang, Dave wrote:
> On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
>> When the link goes down, the link_is_up flag did not return to
>> false. This could have caused some subtle corner case bugs
>> when the link goes up and down quickly.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> 
> Acked-by: Dave Jiang <dave.jiang@intel.com>
> 
> And all the other ntb_perf patches since there were no additional
> changes. 
> 
>> ---
>>  drivers/ntb/test/ntb_perf.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/ntb/test/ntb_perf.c
>> b/drivers/ntb/test/ntb_perf.c
>> index f0784e5..ae9d1b2 100644
>> --- a/drivers/ntb/test/ntb_perf.c
>> +++ b/drivers/ntb/test/ntb_perf.c
>> @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct work_struct
>> *work)
>>  
>>  	if (!perf->link_is_up)
>>  		cancel_delayed_work_sync(&perf->link_work);
>> +
>> +	perf->link_is_up = false;
>>  }
>>  
>>  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
>> -- 
>> 2.1.4

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

#1423554

On Wed, 2016-06-15 at 16:20 -0600, Logan Gunthorpe wrote:
> Hey,
> 
> Actually, I have to retract this patch. After some more thorough
> testing
> I'm finding an issue:
> 
> When you remove and re-install the ntb_perf module very quickly,
> ntb_perf will occasionally miss the link up event. This is because
> the
> link_cleanup work gets delayed long enough that it gets scheduled
> after
> the link up event gets sent. It then cancels the link work that
> should
> have occurred. Without this patch, it never happens because
> link_is_up
> never returns to false.
> 
> I think the correct solution is to just remove the link_cleanup work
> and
> do those actions immediately on receipt of the event. If there's
> agreement on this I can re-spin it again.

I'm ok with that. This is not an issue with ntb_transport?

> 
> Thanks,
> 
> Logan
> 
> 
> On 15/06/16 03:33 PM, Jiang, Dave wrote:
> > 
> > On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
> > > 
> > > When the link goes down, the link_is_up flag did not return to
> > > false. This could have caused some subtle corner case bugs
> > > when the link goes up and down quickly.
> > > 
> > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> > Acked-by: Dave Jiang <dave.jiang@intel.com>
> > 
> > And all the other ntb_perf patches since there were no additional
> > changes. 
> > 
> > > 
> > > ---
> > >  drivers/ntb/test/ntb_perf.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/ntb/test/ntb_perf.c
> > > b/drivers/ntb/test/ntb_perf.c
> > > index f0784e5..ae9d1b2 100644
> > > --- a/drivers/ntb/test/ntb_perf.c
> > > +++ b/drivers/ntb/test/ntb_perf.c
> > > @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct
> > > work_struct
> > > *work)
> > >  
> > >  	if (!perf->link_is_up)
> > >  		cancel_delayed_work_sync(&perf->link_work);
> > > +
> > > +	perf->link_is_up = false;
> > >  }
> > >  
> > >  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx
> > > *perf)
> > > -- 
> > > 2.1.4

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

#1423571

Hey,

On 15/06/16 04:24 PM, Jiang, Dave wrote:
> On Wed, 2016-06-15 at 16:20 -0600, Logan Gunthorpe wrote:
>> Hey,
>>
>> Actually, I have to retract this patch. After some more thorough
>> testing
>> I'm finding an issue:
>>
>> When you remove and re-install the ntb_perf module very quickly,
>> ntb_perf will occasionally miss the link up event. This is because
>> the
>> link_cleanup work gets delayed long enough that it gets scheduled
>> after
>> the link up event gets sent. It then cancels the link work that
>> should
>> have occurred. Without this patch, it never happens because
>> link_is_up
>> never returns to false.
>>
>> I think the correct solution is to just remove the link_cleanup work
>> and
>> do those actions immediately on receipt of the event. If there's
>> agreement on this I can re-spin it again.
> 
> I'm ok with that. This is not an issue with ntb_transport?

Looks like I can get something similar to happen in ntb_transport.
However, it's much rarer and takes significantly more tries to get it to
occur. It does appear to correctly set its link_is_up to false when the
link goes down.

I'm not sure I'm quite clear on the flow in ntb_transport and don't have
time right now to study it so I'll have to let that be someone else's
(fairly minor) issue.

Logan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web