From nobody Sat Aug 12 15:42:21 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RNQ0C0WPDz4qBwk for ; Sat, 12 Aug 2023 15:42:35 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RNQ0B4mMRz3Fp8 for ; Sat, 12 Aug 2023 15:42:34 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pl1-x632.google.com with SMTP id d9443c01a7336-1bdb7b0c8afso8254565ad.3 for ; Sat, 12 Aug 2023 08:42:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kev009.com; s=google; t=1691854953; x=1692459753; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=4fuxRkQ25YJ6NvIH+gDbi/p/xLibvyOMTkv+hqWDWhU=; b=fMoUYmjIo4KZjaVDvOXb50liFuA9ZL3fJtaKcI2y917EiHvCukCErpao50egXpy7qM MlAcEvBrE1w8I+DKOR+RCkN2VZT7484ITMrUxsiURkc3l4doHO+AyD5jE9VIt2j3oTrW tscqfOK4il82p2X4v4kfqQwJ1XdWrjJFjaDNI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691854953; x=1692459753; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4fuxRkQ25YJ6NvIH+gDbi/p/xLibvyOMTkv+hqWDWhU=; b=kAaG/blXOhSZXyypWR88pntLyFOrgXqF7trZSAQQ+ponGxf3L9pJM5gMsmOmoQkf4e u3af/kZWV1EdWyomg5tcr+I+ELcIuB7Nphl9XgAnEwBr4GcN8W6SSkdZrL1nJ7DyYGAn unCKU2SclZH5qrr9lMR3YzQ1ZsoM/RC3e5b6mCERJ1iayzGWHuFfqY35ZXnF47WR63qD oOYEhziswovdKq5yrVQuPmWRTeE8FTSK2c/vAs25g4s5Y1MBggwU9SG/E9Vt2lKoRKFZ hELjzvFeOderrwCn6BOwjEE7kbLQY+mOd4+ZgwItSY8QP2SfGJh0z9iBzW7iKnPX9MQi MpyQ== X-Gm-Message-State: AOJu0Ywc/PBBCU4169NdYACzLXGGYqpdawiv2U1g3xceZU8Qkond8kYg spsfkaaYc93hy+k7SadfvqKwzcH9WPK6Qejve2pqXcvix0l1QVb7xNY= X-Google-Smtp-Source: AGHT+IE3qR9BDjsbXXv9G3cs8Jnr/3Cmpqmk/iNIc7ElHFX0d2eVU07SaiI3mC3ZTlfgehYCpT04Eb+Hmvnm5eHm/9Q= X-Received: by 2002:a17:902:db05:b0:1bb:c971:ef92 with SMTP id m5-20020a170902db0500b001bbc971ef92mr4006139plx.59.1691854952830; Sat, 12 Aug 2023 08:42:32 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202308112251.37BMpOnC064624@gitrepo.freebsd.org> In-Reply-To: From: Kevin Bowling Date: Sat, 12 Aug 2023 08:42:21 -0700 Message-ID: Subject: Re: git: 5f11a33ceeb3 - main - if_vlan: do not enable LRO for bridge interaces To: Kristof Provost Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4RNQ0B4mMRz3Fp8 X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] On Sat, Aug 12, 2023 at 1:45=E2=80=AFAM Kristof Provost wr= ote: > > I also don=E2=80=99t have a setup to reproduce the issues Paul reported. > Given the discussion in the PR I assumed everyone was happy with the fix = he posted. Ok, I happened to be looking into it when the commit went in. Please revert, it's not the right fix. I have tested my patch as such: ifconfig bridge0 create ifconfig em3.11 create ifconfig bridge0 addm em3.11 You will see the em3.11 automagically has LRO removed and the "can't disable some capabilities" message is not present. ifconfig bridge0 deletem em3.11 LRO is restored to em3.11. My patch has the correct functionality intended by the masking code in if_bridge and my patch also fixes the case of disjoint interface capabilities in a bridge. The subtle bug was that vlan_capabilities() in if_vlan was not obeying the requested mask from its IFCAP ioctl. > Best regards, > Kristof Cheers, Kevin > On 12 Aug 2023, at 3:28, Kevin Bowling wrote: > > I think this may be a better fix, currently untested > > > > diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c > > index 6aa872a19364..51f0bc63ebda 100644 > > --- a/sys/net/if_vlan.c > > +++ b/sys/net/if_vlan.c > > @@ -2074,7 +2074,7 @@ vlan_capabilities(struct ifvlan *ifv) > > if (p->if_capabilities & IFCAP_VLAN_HWCSUM) > > cap |=3D p->if_capabilities & IFCAP_LRO; > > if (p->if_capenable & IFCAP_VLAN_HWCSUM) > > - ena |=3D p->if_capenable & IFCAP_LRO; > > + ena |=3D mena & IFCAP_LRO; > > > > /* > > * If the parent interface can offload TCP connections over VLAN= s then > > > > On Fri, Aug 11, 2023 at 5:07=E2=80=AFPM Kevin Bowling wrote: > >> > >> I am wondering what is going on with this patch, it doesn't seem like > >> a full fix. At the very least we should clean up the "can't disable > >> some capabilities" message. > >> > >> I've been looking at this for a couple hours and am wondering what is > >> going on in if_bridge.. it looks like it correctly loops over the > >> interfaces and masks ifcaps it doesn't like using the interface's > >> ioctl SIOCSIFCAP. A similar pattern is used in if_lagg. I don't see > >> any issue there so far. I also don't see the SIOCSICAP ioctl failing > >> nor did vixie's message report that. > >> > >> I checked on a cxgbe(9) bridged vlan setup I have and see this message > >> "bridge0: can't disable some capabilities on vlan0: 0x400" > >> > >> So I am wondering if the bug is that if_vlan SIOCSIFCAP is > >> disregarding the ioctl's request mask.. it is just blindly calling > >> vlan_capabilities. > >> > >> Regards, > >> Kevin > >> > >> > >> On Fri, Aug 11, 2023 at 3:51=E2=80=AFPM Kristof Provost wrote: > >>> > >>> The branch main has been updated by kp: > >>> > >>> URL: https://cgit.FreeBSD.org/src/commit/?id=3D5f11a33ceeb385477cb22d= 9ad5941061c5a26be9 > >>> > >>> commit 5f11a33ceeb385477cb22d9ad5941061c5a26be9 > >>> Author: Paul Vixie > >>> AuthorDate: 2023-08-11 18:17:16 +0000 > >>> Commit: Kristof Provost > >>> CommitDate: 2023-08-11 22:50:37 +0000 > >>> > >>> if_vlan: do not enable LRO for bridge interaces > >>> > >>> If the parent interface is not a bridge and can do LRO and > >>> checksum offloading on VLANs, then guess it may do LRO on VLANs. > >>> False positive here cost nothing, while false negative may lead > >>> to some confusions. According to Wikipedia: > >>> > >>> "LRO should not operate on machines acting as routers, as it brea= ks > >>> the end-to-end principle and can significantly impact performance= ." > >>> > >>> The same reasoning applies to machines acting as bridges. > >>> > >>> PR: 254596 > >>> MFC after: 3 weeks > >>> --- > >>> sys/net/if_vlan.c | 22 +++++++++++++++------- > >>> 1 file changed, 15 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c > >>> index 6aa872a19364..92e4e4247e3d 100644 > >>> --- a/sys/net/if_vlan.c > >>> +++ b/sys/net/if_vlan.c > >>> @@ -2067,14 +2067,22 @@ vlan_capabilities(struct ifvlan *ifv) > >>> } > >>> > >>> /* > >>> - * If the parent interface can do LRO and checksum offloading= on > >>> - * VLANs, then guess it may do LRO on VLANs. False positive = here > >>> - * cost nothing, while false negative may lead to some confus= ions. > >>> + * If the parent interface is not a bridge and can do LRO and > >>> + * checksum offloading on VLANs, then guess it may do LRO on V= LANs. > >>> + * False positive here cost nothing, while false negative may = lead > >>> + * to some confusions. According to Wikipedia: > >>> + * > >>> + * "LRO should not operate on machines acting as routers, as i= t breaks > >>> + * the end-to-end principle and can significantly impact perfo= rmance." > >>> + * > >>> + * The same reasoning applies to machines acting as bridges. > >>> */ > >>> - if (p->if_capabilities & IFCAP_VLAN_HWCSUM) > >>> - cap |=3D p->if_capabilities & IFCAP_LRO; > >>> - if (p->if_capenable & IFCAP_VLAN_HWCSUM) > >>> - ena |=3D p->if_capenable & IFCAP_LRO; > >>> + if (ifp->if_bridge =3D=3D NULL) { > >>> + if (p->if_capabilities & IFCAP_VLAN_HWCSUM) > >>> + cap |=3D p->if_capabilities & IFCAP_LRO; > >>> + if (p->if_capenable & IFCAP_VLAN_HWCSUM) > >>> + ena |=3D p->if_capenable & IFCAP_LRO; > >>> + } > >>> > >>> /* > >>> * If the parent interface can offload TCP connections over V= LANs then > >>>