RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 1023503 - Port RHEL 6.5 PPPoE reconnect delay to git master
Summary: Port RHEL 6.5 PPPoE reconnect delay to git master
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Jirka Klimes
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-10-25 14:48 UTC by Dan Williams
Modified: 2014-06-18 02:48 UTC (History)
6 users (show)

Fixed In Version: NetworkManager-0.9.9.0-28.git20131212.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-06-13 10:18:18 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
rh602265-pppoe-reconnect-delay.patch (4.99 KB, patch)
2013-10-25 14:49 UTC, Dan Williams
no flags Details | Diff
[PATCH] ethernet: add reconnect delay for PPPoE connections (for master) (5.06 KB, patch)
2013-12-09 09:15 UTC, Jirka Klimes
no flags Details | Diff
[PATCH v2] ethernet: add reconnect delay for PPPoE connections (rh #602265) (5.28 KB, patch)
2013-12-11 11:47 UTC, Jirka Klimes
no flags Details | Diff
[PATCH] fixup! ethernet: add reconnect delay for PPPoE connections (rh #602265) (7.21 KB, patch)
2013-12-11 12:14 UTC, Thomas Haller
no flags Details | Diff

Description Dan Williams 2013-10-25 14:48:25 UTC
We did a patch for 6.5 to add a reconnect delay when auto-reconnecting a PPPoE connection, becuase often the PPP peer on the other end of the link needs a couple seconds to notice that the local machine has terminated the connection and to get ready to reconnect.  For some details on that, see bug #602265.

The patch introduces a short delay during stage1 of device activation if the PPPoE was run on this device within the last 7 seconds.  This allows the peer time to clean up from the connection and get ready for the reconnect.

Comment 1 Dan Williams 2013-10-25 14:49:22 UTC
Created attachment 816194 [details]
rh602265-pppoe-reconnect-delay.patch

Comment 3 Jirka Klimes 2013-12-09 09:15:47 UTC
Created attachment 834237 [details]
[PATCH]  ethernet: add reconnect delay for PPPoE connections (for master)

Previous patch rebased to upstream master. (You can also see it in jk/rh1023503-pppoe).

Comment 4 Dan Williams 2013-12-09 15:03:44 UTC
Looks good to me; I can test it tomorrow, but if you want to test it out yourself, you can install the rp-pppoe package on a different machine on the same LAN and then follow these directions:

http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server-in-linux.html

starting at the "Let's proceed to configure the machines one by one." part, and stopping at the "For the pppoe client machine, the important configuration files are" part since NetworkManager handles that.  Then you can run the pppoe server with a command like:

pppoe-server -k -I eth0 -F

and then let NetworkManager attempt to connect.  As long as the two machines are on the same LAN, the client will broadcast for a PPPoE server, the server will reply, and they will negotiate the connection.  Then you could do something like "nmcli dev eth0 disconnect; nmcli con up my-pppoe-test" and make sure you see the log messages about waiting.

Comment 5 Thomas Haller 2013-12-10 13:01:46 UTC
(In reply to Jirka Klimes from comment #3)
> Created attachment 834237 [details]
> [PATCH]  ethernet: add reconnect delay for PPPoE connections (for master)
> 
> Previous patch rebased to upstream master. (You can also see it in
> jk/rh1023503-pppoe).

I think, using time() is wrong, because it's affected by daylight saving time and resetting of the clock. I'd add a function nm_utils_get_monotonic_timestamp_ns(), that should be used whenever calculating time offsets.

See th/review/jk/rh1023503-pppoe. It includes two minor fixups.

Comment 6 Thomas Haller 2013-12-10 13:04:26 UTC
(In reply to Thomas Haller from comment #5)

> I think, using time() is wrong, because it's affected by daylight saving
> time and resetting of the clock.

aehh..., partly wrong. time() returns UTC, so no problem with DST.

Comment 7 Dan Williams 2013-12-10 17:00:16 UTC
(In reply to Thomas Haller from comment #6)
> (In reply to Thomas Haller from comment #5)
> 
> > I think, using time() is wrong, because it's affected by daylight saving
> > time and resetting of the clock.
> 
> aehh..., partly wrong. time() returns UTC, so no problem with DST.

Yeah, it might be wrong if the system suspends; so technically it should be something like clock_gettime(CLOCK_MONOTONIC).  Though in this case it shouldn't be too large of an issue since it's a very short timeout, and the systems that are running PPPoE here won't usually be suspended.  I'm fine with either option: (a) change to use clock_gettime() or (b) leave as-is for now.

Comment 8 Jirka Klimes 2013-12-11 11:43:12 UTC
(In reply to Dan Williams from comment #4)
> Looks good to me; I can test it tomorrow, but if you want to test it out
> yourself, you can install the rp-pppoe package on a different machine on the
> same LAN and then follow these directions:
> 
> http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server-
> in-linux.html
> 
Dan, thanks for the testing instructions. It helped to set the testing environment a lot.

I've found that there was a problem setting last_pppoe_time in device_state_changed(), because in that function activation request is already NULL and thus device_get_setting (dev, NM_TYPE_SETTING_PPPOE) returns NULL. So I moved setting last_pppoe_time to deactivate(), which works good, because it is called *before* clear_act_request() in device.c:nm_device_deactivate().

Also, I adapted the delay in g_timeout_add_seconds() to be the difference between now and the last time, because we don't probably want to wait the whole PPPOE_RECONNECT_DELAY interval, but only the remaining time.

As for time changes Thomas proposes, I would prefer not to do changes for this patch. Rather we could adapt timestamp handling in a separate branch for whole NM if we decide to go that way.

Comment 9 Jirka Klimes 2013-12-11 11:47:37 UTC
Created attachment 835238 [details]
[PATCH v2] ethernet: add reconnect delay for PPPoE connections (rh #602265)

Version 2 of the previous patch. Changes:
* setting last_pppoe_time in deactivate()
* waiting only remaining time of PPPOE_RECONNECT_DELAY

The updated patch is also at upstream branch jk/rh1023503-pppoe.

Comment 10 Jirka Klimes 2013-12-11 11:55:01 UTC
Updated commit message in the branch to reference bug 1023503 rather than 602265.

Comment 11 Thomas Haller 2013-12-11 12:14:20 UTC
Created attachment 835245 [details]
[PATCH] fixup! ethernet: add reconnect delay for PPPoE connections (rh #602265)

(In reply to Jirka Klimes from comment #8)

> As for time changes Thomas proposes, I would prefer not to do changes for
> this patch. Rather we could adapt timestamp handling in a separate branch
> for whole NM if we decide to go that way.

well, ok about the usage of time() or not, but what about the other bits?

Notice, that the commit message does not reference this bug 1023503, but bug 602265.

Comment 12 Dan Williams 2013-12-11 16:52:47 UTC
Current branch works well in my test setup.

Thomas' fixups look good and work correctly, with the exception to the changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the last connection was PPPoE but this connection is not PPPoE.  That could be changed to:

if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)

Comment 13 Thomas Haller 2013-12-11 17:08:26 UTC
(In reply to Dan Williams from comment #12)
> Current branch works well in my test setup.
> 
> Thomas' fixups look good and work correctly, with the exception to the
> changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a
> normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the
> last connection was PPPoE but this connection is not PPPoE.  That could be
> changed to:
> 
> if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)



Actually, it _would be_ correct though, because last_pppoe_time only gets set, if the device has such a setting. 
+        if (device_get_setting (device, NM_TYPE_SETTING_PPPOE))
+            NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time (NULL);

But it might be more explicit to check it again.

I would propose a different order:
 if (priv->last_pppoe_time && device_get_setting (dev, NM_TYPE_SETTING_PPPOE))
because the previous is much cheaper to evaluate.

Comment 14 Thomas Haller 2013-12-11 17:09:52 UTC
(In reply to Thomas Haller from comment #13)
> (In reply to Dan Williams from comment #12)
> > Current branch works well in my test setup.
> > 
> > Thomas' fixups look good and work correctly, with the exception to the
> > changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a
> > normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the
> > last connection was PPPoE but this connection is not PPPoE.  That could be
> > changed to:
> > 
> > if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)
> 
> 
> 
> Actually, it _would be_ correct though, because last_pppoe_time only gets
> set, if the device has such a setting. 
> +        if (device_get_setting (device, NM_TYPE_SETTING_PPPOE))
> +            NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time
> (NULL);
> 
> But it might be more explicit to check it again.
> 
> I would propose a different order:
>  if (priv->last_pppoe_time && device_get_setting (dev,
> NM_TYPE_SETTING_PPPOE))
> because the previous is much cheaper to evaluate.

Ok, you are right :)
It is really needed. Never mind :D

Comment 15 Dan Williams 2013-12-11 17:20:33 UTC
Cheaper order is fine with me.

Comment 17 Jirka Klimes 2013-12-11 17:49:34 UTC
Fix pushed to upstream master:
5fad262 ethernet: add reconnect delay for PPPoE connections (rh #1023503)

Comment 20 Ludek Smid 2014-06-13 10:18:18 UTC
This request was resolved in Red Hat Enterprise Linux 7.0.

Contact your manager or support representative in case you have further questions about the request.


Note You need to log in before you can comment on or make changes to this bug.