Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tap(4): allow full-duplex and non-zero speed #745

Closed
wants to merge 1 commit into from

Conversation

igalic
Copy link
Contributor

@igalic igalic commented May 18, 2023

tap(4) devices advertise themselves as just 'ethernet autoselect', without duplex or speed capabilities.
This advertisement makes them unable to be aggregated into lacp-based lagg(4):

  • lacp code requires underlying interfaces to be full-duplex, else interface will not participate in lacp at all
  • lacp code requires underlying interface to have non-zero speed, else this interface can not be selected as active aggregator

PR#: 217374
Reported by: Alexandre Snarskii [email protected]
Co-authored-by: Mina Galić [email protected]

@igalic
Copy link
Contributor Author

igalic commented May 18, 2023

hey @snar ; i just found your Patch in Bugzilla and thought resurrecting it here would be better.

@emaste
Copy link
Member

emaste commented May 18, 2023

If this is an updated version of the patch originally submitted in the PR then we ought to use the Co-authored-by: Name <email> trailer

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

@igalic
Copy link
Contributor Author

igalic commented May 18, 2023

@emaste all I've done is moved the original author's patch into the new file.
but I'll happily add myself as Co-Author for doing the work

tap(4) devices advertise themselves as just 'ethernet autoselect',
without duplex or speed capabilities.
This advertisement makes them unable to be aggregated into lacp-based
lagg(4):
- lacp code requires underlying interfaces to be full-duplex, else
  interface will not participate in lacp at all
- lacp code requires underlying interface to have non-zero speed, else
  this interface can not be selected as active aggregator

PR#: 217374
Reported by: Alexandre Snarskii <[email protected]>
Co-authored-by: Mina Galić <[email protected]>
Copy link
Contributor

@gmshake gmshake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically looks good to me.

@@ -1341,7 +1341,7 @@ tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
dummy = ifmr->ifm_count;
ifmr->ifm_count = 1;
ifmr->ifm_status = IFM_AVALID;
ifmr->ifm_active = IFM_ETHER;
ifmr->ifm_active = IFM_ETHER | IFM_FDX | IFM_1000_T;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer 10Gbps, although higher is also possible but this keeps align with some virtual interfaces such as epair.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

netgtaph and ether switch use IFM_1000_T, so we should bump then as well

(and vxlan doesn't set a limit… hmmmm)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common frontend vtnet (virtio-net) advertise it as Ethernet autoselect (10Gbase-T <full-duplex>) and there is no choices for supported media, so I think it should be IFM_10G_T .

root@freebsd:~ # ifconfig -m vtnet0
vtnet0: flags=8863<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        options=80028<VLAN_MTU,JUMBO_MTU,LINKSTATE>
        capabilities=90028<VLAN_MTU,JUMBO_MTU,VLAN_HWFILTER,LINKSTATE>
        ether 52:54:00:12:34:56
        inet 192.168.99.31 netmask 0xffffff00 broadcast 192.168.99.255
        media: Ethernet autoselect (10Gbase-T <full-duplex>)
        status: active
        supported media:
                media autoselect
        nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>

I'll test and report later.

Copy link
Contributor

@gmshake gmshake May 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test looks good, except the default if_baudrate (of tap interface) is not set correspondingly.

# ifconfig
tap0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
	options=80000<LINKSTATE>
	ether 8a:d6:1b:7f:b6:28
	groups: tap
	media: Ethernet 10Gbase-T <full-duplex>
	status: active
	nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
	Opened by PID 56600
tap1: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
	options=80000<LINKSTATE>
	ether 8a:d6:1b:7f:b6:28
	hwaddr 6e:b3:f0:23:3d:9c
	groups: tap
	media: Ethernet 10Gbase-T <full-duplex>
	status: active
	nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
	Opened by PID 56600
lagg0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
	options=80000<LINKSTATE>
	ether 8a:d6:1b:7f:b6:28
	inet 192.168.199.1 netmask 0xffffff00 broadcast 192.168.199.255
	laggproto lacp lagghash l2,l3,l4
	laggport: tap0 flags=1c<ACTIVE,COLLECTING,DISTRIBUTING>
	laggport: tap1 flags=1c<ACTIVE,COLLECTING,DISTRIBUTING>
	groups: lagg
	media: Ethernet autoselect
	status: active
	nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ng does a much better job doing all of this, but it also sets every negotiable speed.

I also don't see how to access ifmedia from the point at which baudrate is set in tuntap.

I think this might need more refactoring to do right then.

freebsd-git pushed a commit that referenced this pull request May 31, 2023
tap(4) devices advertise themselves as just 'ethernet autoselect',
without duplex or speed capabilities.
This advertisement makes them unable to be aggregated into lacp-based
lagg(4):
- lacp code requires underlying interfaces to be full-duplex, else
  interface will not participate in lacp at all
- lacp code requires underlying interface to have non-zero speed, else
  this interface can not be selected as active aggregator

PR: 217374
Reported-by: Alexandre Snarskii <[email protected]>
Co-authored-by: Mina Galić <[email protected]>
Reviewed-by: imp,karles
Pull-request: #745
@bsdimp
Copy link
Member

bsdimp commented May 31, 2023

I've landed this patch, even though it's imperfect in the review. It helps many things, though a future version could help even more. I'll let that future version be submitted when it's ready.

@bsdimp bsdimp closed this May 31, 2023
@igalic igalic deleted the tuntap/full-duplex branch May 31, 2023 17:39
@emaste emaste added the merged label Jun 12, 2023
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 11, 2023
tap(4) devices advertise themselves as just 'ethernet autoselect',
without duplex or speed capabilities.
This advertisement makes them unable to be aggregated into lacp-based
lagg(4):
- lacp code requires underlying interfaces to be full-duplex, else
  interface will not participate in lacp at all
- lacp code requires underlying interface to have non-zero speed, else
  this interface can not be selected as active aggregator

PR: 217374
Reported-by: Alexandre Snarskii <[email protected]>
Co-authored-by: Mina Galić <[email protected]>
Reviewed-by: imp,karles
Pull-request: freebsd/freebsd-src#745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants