Page MenuHomeFreeBSD

dhclient: add support for interface-mtu (26)
ClosedPublic

Authored by novel on Mar 18 2016, 6:19 PM.

Details

Summary

Make dhclient set interface MTU if it was provided.

This version implements MTU setting in dhclient itself before it runs dhclient-script.

Also, to be able to test this with devd, I've modified if_vtnet(4) to send LINK_(UP|DOWN) (inl. the case when MTU is being changed): https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209427

So far it works well in my setup...

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

novel updated this revision to Diff 14425.Mar 18 2016, 6:19 PM
novel retitled this revision from to dhclient-script: add support for interface-mtu (26).
novel updated this object.
novel edited the test plan for this revision. (Show Details)
novel added a reviewer: Src Committers.
novel set the repository for this revision to rS FreeBSD src repository.
ian added a reviewer: ian.Mar 21 2016, 4:50 PM
ian added a subscriber: ian.
ian added inline comments.
dhclient-script
263 ↗(On Diff #14425)

I think this line should probably be:

if [ -n "$new_interface_mtu" -a "$old_interface_mtu" != "$new_interface_mtu" ]; then

That should prevent spamming syslog with non-changes on lease renewal (but I don't have any easy way to test the change).

novel updated this revision to Diff 14538.Mar 23 2016, 4:40 AM

Update the check to not set MTU if it didn't change on renewal.

novel marked an inline comment as done.Mar 23 2016, 4:43 AM
novel added inline comments.
dhclient-script
263 ↗(On Diff #14425)

Yeah, you're right. I've updated the diff and now it doesn't re-set MTU if not needed. I'm not sure how to trigger renewal with MTU change in my env either.

ae added a subscriber: ae.Mar 30 2016, 10:22 AM

When I tested similar patch I found the problem described here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206721
I tested with em(4).

novel marked an inline comment as done.Mar 30 2016, 10:44 AM
In D5675#123498, @ae wrote:

When I tested similar patch I found the problem described here https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206721
I tested with em(4).

Hm... I'm testing this with vtnet(4). It looks something like this:

Mar 30 10:35:52 host-10-20-0-5 dhclient: New MTU (vtnet0): 1458
Mar 30 10:35:52 host-10-20-0-5 dhclient: New IP Address (vtnet0): 10.20.0.5
Mar 30 10:35:52 host-10-20-0-5 dhclient: New Subnet Mask (vtnet0): 255.255.255.0
Mar 30 10:35:52 host-10-20-0-5 dhclient: New Broadcast Address (vtnet0): 10.20.0.255
Mar 30 10:35:52 host-10-20-0-5 dhclient: New Routers (vtnet0): 10.20.0.1
Mar 30 10:35:52 host-10-20-0-5 dhclient: New Classless Static Routes (vtnet0):  169.254.169.254/32 10.20.0.1 default 10.20.0.1

I do see interface going up and down for a moment, however it does not cause dhclient restart.

E.g., you can see that pids are the same:

$ date
Wed Mar 30 10:41:43 UTC 2016
$ ps aux|grep dhclient|grep -v grep
root    45306   0.0  0.0   10588   2480  -  Ss   10:35       0:00.06 dhclient: vtnet0 [priv] (dhclient)
_dhcp   45328   0.0  0.0   10588   2660  -  Ss   10:35       0:00.01 dhclient: vtnet0 (dhclient)
$ date
Wed Mar 30 10:42:59 UTC 2016
$ ps aux|grep dhclient|grep -v grep
root    45306   0.0  0.0   10588   2480  -  Is   10:35       0:00.07 dhclient: vtnet0 [priv] (dhclient)
_dhcp   45328   0.0  0.0   10588   2660  -  Is   10:35       0:00.01 dhclient: vtnet0 (dhclient)
$

I don't see new records in logs either.

I run it either like "dhclient -d vtnet0" or "dhclient vtnet0". Maybe some additional steps are required to reproduce that?

ae added a comment.Mar 30 2016, 11:10 AM

I think this is specific to each network driver. if_em(4) does hardware reset on SIOCSIFMTU. As I can see, ixgbe(4) also does the same and changing MTU initiates link state change.

Mar 30 15:16:46 test25 devd: Processing event '!system=IFNET subsystem=ix0 type=LINK_DOWN'
Mar 30 15:16:46 test25 devd: Pushing table
Mar 30 15:16:46 test25 devd: Processing notify event
Mar 30 15:16:46 test25 devd: Popping table
Mar 30 15:16:46 test25 devd: Processing event '!system=IFNET subsystem=ix0 type=LINK_UP'
novel added a comment.EditedMar 31 2016, 6:40 AM
In D5675#123508, @ae wrote:

I think this is specific to each network driver. if_em(4) does hardware reset on SIOCSIFMTU. As I can see, ixgbe(4) also does the same and changing MTU initiates link state change.

Mar 30 15:16:46 test25 devd: Processing event '!system=IFNET subsystem=ix0 type=LINK_DOWN'
Mar 30 15:16:46 test25 devd: Pushing table
Mar 30 15:16:46 test25 devd: Processing notify event
Mar 30 15:16:46 test25 devd: Popping table
Mar 30 15:16:46 test25 devd: Processing event '!system=IFNET subsystem=ix0 type=LINK_UP'

Hm, yes, I don't see events like that from devd, though I can see ping interrupts for a moment:

64 bytes from 10.20.0.1: icmp_seq=2 ttl=64 time=0.435 ms
64 bytes from 10.20.0.1: icmp_seq=3 ttl=64 time=0.453 ms
ping: sendto: No route to host
ping: sendto: No route to host
64 bytes from 10.20.0.1: icmp_seq=6 ttl=64 time=0.467 ms
64 bytes from 10.20.0.1: icmp_seq=7 ttl=64 time=0.352 ms

BTW, I'm wondering if the issue you're seeing is caused by dhclient alone or because devd contains the dhclient start rule when interface goes up?

Anyway, I need to find some way to reproduce it myself...

On a related note, I'm wondering if vtnet(4) should spawn LINK_(UP|DOWN) events?

ae added a subscriber: jhb.Mar 31 2016, 9:10 AM
novel updated this revision to Diff 17038.May 28 2016, 6:01 PM
novel retitled this revision from dhclient-script: add support for interface-mtu (26) to dhclient: add support for interface-mtu (26).
novel updated this object.
cem accepted this revision.Jun 7 2016, 6:47 PM
cem added a reviewer: cem.
cem added a subscriber: cem.

LGTM other than some nits.

sbin/dhclient/dhclient.c
809–810 ↗(On Diff #17038)

mtu = be16dec(opt->data); ?

811 ↗(On Diff #17038)

magic number?

This revision is now accepted and ready to land.Jun 7 2016, 6:47 PM
novel updated this revision to Diff 17451.Jun 9 2016, 6:11 AM
novel edited edge metadata.
This revision now requires review to proceed.Jun 9 2016, 6:11 AM
novel marked 2 inline comments as done.Jun 9 2016, 6:19 AM

Thanks for taking a look, Conrad. I've updated the diff based on your suggestions.

cem accepted this revision.Jun 9 2016, 6:54 AM
cem edited edge metadata.
cem added inline comments.
sbin/dhclient/dhclient.c
8–9 ↗(On Diff #17451)

mismerge?

16–17 ↗(On Diff #17451)

int void looks like a mis-merge

29 ↗(On Diff #17451)

(unsigned)mtu to match the %u format.

This revision is now accepted and ready to land.Jun 9 2016, 6:54 AM
cem added a comment.Jun 9 2016, 6:56 AM

Ignore the mismerge comments. I think phabricator is just confused by the lack of context. For reference, phabricator does better with full context (diff -U999999999).

novel updated this revision to Diff 17452.Jun 9 2016, 7:03 AM
novel edited edge metadata.

New diff fixing the %u (unsinged)mtu bit and preserving context.

This revision now requires review to proceed.Jun 9 2016, 7:03 AM
cem accepted this revision.Jun 9 2016, 3:45 PM
cem edited edge metadata.
This revision is now accepted and ready to land.Jun 9 2016, 3:45 PM
sbruno edited edge metadata.Jun 9 2016, 8:16 PM
sbruno added a subscriber: rstone.
novel added a comment.Jun 23 2016, 5:54 PM

@ae could you please test the latest patch in your environment?

I've tested it, but I only can test it with vtnet, and also I have to use additional patch to make it send devctl events (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209427).

Now when HEAD appears to be open, this change could probably go in. However, as I don't have src commit bit, it seems that I either need to get a explicit approval or somebody else committing this.

Anyway, now when bhyve got e1000 support, I can test thing change not only with my patches virtio-net, so I'll try to test it to re-confirm it addresses concerns outlined by @ae.

In D5675#149197, @novel wrote:

Now when HEAD appears to be open, this change could probably go in. However, as I don't have src commit bit, it seems that I either need to get a explicit approval or somebody else committing this.
Anyway, now when bhyve got e1000 support, I can test thing change not only with my patches virtio-net, so I'll try to test it to re-confirm it addresses concerns outlined by @ae.

e1000 support went in yesterday: https://svnweb.freebsd.org/base?view=revision&revision=302504

novel added a comment.Jul 11 2016, 1:29 AM
In D5675#149197, @novel wrote:

Now when HEAD appears to be open, this change could probably go in. However, as I don't have src commit bit, it seems that I either need to get a explicit approval or somebody else committing this.
Anyway, now when bhyve got e1000 support, I can test thing change not only with my patches virtio-net, so I'll try to test it to re-confirm it addresses concerns outlined by @ae.

e1000 support went in yesterday: https://svnweb.freebsd.org/base?view=revision&revision=302504

yeah, that's what I'm planning to use to test the devctl thing, because I had to patch virtio to send these events (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209427), but that's probably not the best testing environment...

cem added a comment.Jul 11 2016, 4:52 AM

I can commit it for you, but I'd like wider review (i.e. someone other than me signing off on it) before I do so.

In D5675#149247, @cem wrote:

I can commit it for you, but I'd like wider review (i.e. someone other than me signing off on it) before I do so.

Hi,

This is something I'm just looking into again now. I have been after this since the ixv driver started working on AWS EC2
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206721

If testing by me would help get this in for 11, please let me know.

Regards Jarrod

jhb added a comment.Sep 2 2016, 3:34 PM

I think the issues ae@ was referring to were resolved by having dhclient not drop the interface's configuration when the link goes down. Thus, during a link flap the interface remains configured and continues to work. (See r239564)

cem added a comment.Sep 2 2016, 3:46 PM

I think we've missed the boat on 11.0. 12 is the current branch I would land this in. It could be MFCed for 11.1 by somebody interested. I'd still like someone else's Approval to commit, though :-).

allanjude accepted this revision.Sep 2 2016, 7:38 PM
allanjude added a reviewer: allanjude.

lgtm

This revision was automatically updated to reflect the committed changes.