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

Groups > linux.kernel > #1711466 > unrolled thread

RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer

Started byTom Talpey <ttalpey@microsoft.com>
First post2017-08-14 23:20 +0200
Last post2017-08-15 02:20 +0200
Articles 4 — 2 participants

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 v1] 25/37] [CIFS] SMBD: Support SMBD idle connection  timer Tom Talpey <ttalpey@microsoft.com> - 2017-08-14 23:20 +0200
    RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection  timer Long Li <longli@microsoft.com> - 2017-08-15 01:30 +0200
      RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection  timer Tom Talpey <ttalpey@microsoft.com> - 2017-08-15 01:50 +0200
        RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection  timer Long Li <longli@microsoft.com> - 2017-08-15 02:20 +0200

#1711466 — RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer

FromTom Talpey <ttalpey@microsoft.com>
Date2017-08-14 23:20 +0200
SubjectRE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer
Message-ID<ueuWd-Mv-7@gated-at.bofh.it>
> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> owner@vger.kernel.org] On Behalf Of Long Li
> Sent: Wednesday, August 2, 2017 4:11 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer
> 
> +static int keep_alive_interval = 120;

This is the recommended value, but not the only possibility.

> @@ -1348,6 +1369,10 @@ struct cifs_rdma_info* cifs_create_rdma_session(
>         init_waitqueue_head(&info->wait_send_queue);
>         init_waitqueue_head(&info->wait_reassembly_queue);
> 
> +       INIT_DELAYED_WORK(&info->idle_timer_work, idle_connection_timer);
> +       schedule_delayed_work(&info->idle_timer_work,
> +               info->keep_alive_interval*HZ);
> +

This initialization is ok, but the timer should be rescheduled (extended) any time
any packet is sent. There is no need to perform keepalives on an active SMB Direct
connection.

Tom.

[toc] | [next] | [standalone]

#1711563

FromLong Li <longli@microsoft.com>
Date2017-08-15 01:30 +0200
Message-ID<uewY2-1YL-11@gated-at.bofh.it>
In reply to#1711466

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 2:12 PM
> To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>;
> linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> kernel@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > owner@vger.kernel.org] On Behalf Of Long Li
> > Sent: Wednesday, August 2, 2017 4:11 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>
> > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > timer
> >
> > +static int keep_alive_interval = 120;
> 
> This is the recommended value, but not the only possibility.
> 
> > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> cifs_create_rdma_session(
> >         init_waitqueue_head(&info->wait_send_queue);
> >         init_waitqueue_head(&info->wait_reassembly_queue);
> >
> > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> idle_connection_timer);
> > +       schedule_delayed_work(&info->idle_timer_work,
> > +               info->keep_alive_interval*HZ);
> > +
> 
> This initialization is ok, but the timer should be rescheduled (extended) any
> time any packet is sent. There is no need to perform keepalives on an active
> SMB Direct connection.

My feeling is that rescheduling on a work queue for every packet is sent is not efficient, especially under heavy conditions.

Firing it every 120 seconds doesn't seem to be big waste and may actually save some CPU.

> 
> Tom.

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

#1711569

FromTom Talpey <ttalpey@microsoft.com>
Date2017-08-15 01:50 +0200
Message-ID<uexho-24Q-5@gated-at.bofh.it>
In reply to#1711563
> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> owner@vger.kernel.org] On Behalf Of Long Li
> Sent: Monday, August 14, 2017 7:30 PM
> To: Tom Talpey <ttalpey@microsoft.com>; Steve French <sfrench@samba.org>;
> linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> [This sender failed our fraud detection checks and may not be who they appear
> to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> > -----Original Message-----
> > From: Tom Talpey
> > Sent: Monday, August 14, 2017 2:12 PM
> > To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>;
> > linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> > kernel@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>
> > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > timer
> >
> > > -----Original Message-----
> > > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > > owner@vger.kernel.org] On Behalf Of Long Li
> > > Sent: Wednesday, August 2, 2017 4:11 PM
> > > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org
> > > Cc: Long Li <longli@microsoft.com>
> > > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > > timer
> > >
> > > +static int keep_alive_interval = 120;
> >
> > This is the recommended value, but not the only possibility.
> >
> > > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> > cifs_create_rdma_session(
> > >         init_waitqueue_head(&info->wait_send_queue);
> > >         init_waitqueue_head(&info->wait_reassembly_queue);
> > >
> > > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> > idle_connection_timer);
> > > +       schedule_delayed_work(&info->idle_timer_work,
> > > +               info->keep_alive_interval*HZ);
> > > +
> >
> > This initialization is ok, but the timer should be rescheduled (extended) any
> > time any packet is sent. There is no need to perform keepalives on an active
> > SMB Direct connection.
> 
> My feeling is that rescheduling on a work queue for every packet is sent is not
> efficient, especially under heavy conditions.

That's not what I was suggesting. Cant the timer simply be re-extended to the
120-second interval? I.e. on an active connection, it will never fire because it's
always advancing.

As defined here, it will go off and send a keepalive every 120 seconds. The
idle_connection_timer() routine unconditionally sends it.

> 
> Firing it every 120 seconds doesn't seem to be big waste and may actually save
> some CPU.

Firing the timer, no big deal. Sending the packets and requiring the peer to process
them too, disagree.

Tom.

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

#1711570

FromLong Li <longli@microsoft.com>
Date2017-08-15 02:20 +0200
Message-ID<uexKp-2vI-1@gated-at.bofh.it>
In reply to#1711569

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 4:42 PM
> To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>;
> linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > owner@vger.kernel.org] On Behalf Of Long Li
> > Sent: Monday, August 14, 2017 7:30 PM
> > To: Tom Talpey <ttalpey@microsoft.com>; Steve French
> > <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba-technical@lists.samba.org; linux- kernel@vger.kernel.org
> > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > connection timer
> >
> > [This sender failed our fraud detection checks and may not be who they
> > appear to be. Learn about spoofing at
> > http://aka.ms/LearnAboutSpoofing]
> >
> > > -----Original Message-----
> > > From: Tom Talpey
> > > Sent: Monday, August 14, 2017 2:12 PM
> > > To: Long Li <longli@microsoft.com>; Steve French
> > > <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > > samba-technical@lists.samba.org; linux- kernel@vger.kernel.org
> > > Cc: Long Li <longli@microsoft.com>
> > > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > > connection timer
> > >
> > > > -----Original Message-----
> > > > From: linux-cifs-owner@vger.kernel.org [mailto:linux-cifs-
> > > > owner@vger.kernel.org] On Behalf Of Long Li
> > > > Sent: Wednesday, August 2, 2017 4:11 PM
> > > > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > > > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org
> > > > Cc: Long Li <longli@microsoft.com>
> > > > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > > > connection timer
> > > >
> > > > +static int keep_alive_interval = 120;
> > >
> > > This is the recommended value, but not the only possibility.
> > >
> > > > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> > > cifs_create_rdma_session(
> > > >         init_waitqueue_head(&info->wait_send_queue);
> > > >         init_waitqueue_head(&info->wait_reassembly_queue);
> > > >
> > > > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> > > idle_connection_timer);
> > > > +       schedule_delayed_work(&info->idle_timer_work,
> > > > +               info->keep_alive_interval*HZ);
> > > > +
> > >
> > > This initialization is ok, but the timer should be rescheduled
> > > (extended) any time any packet is sent. There is no need to perform
> > > keepalives on an active SMB Direct connection.
> >
> > My feeling is that rescheduling on a work queue for every packet is
> > sent is not efficient, especially under heavy conditions.
> 
> That's not what I was suggesting. Cant the timer simply be re-extended to
> the 120-second interval? I.e. on an active connection, it will never fire
> because it's always advancing.
> 
> As defined here, it will go off and send a keepalive every 120 seconds. The
> idle_connection_timer() routine unconditionally sends it.
> 
> >
> > Firing it every 120 seconds doesn't seem to be big waste and may
> > actually save some CPU.
> 
> Firing the timer, no big deal. Sending the packets and requiring the peer to
> process them too, disagree.

Fair enough. I will fix the code to modify delayed work instead of firing every 120 seconds.

> 
> Tom.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web