From: Michal Kubecek <mkubecek@suse•cz>
To: Jay Vosburgh <jay.vosburgh@canonical•com>
Cc: David Miller <davem@davemloft•net>,
netdev@vger•kernel.org, vfalico@gmail•com, andy@greyhouse•net,
mirq-linux@rere•qmqm.pl
Subject: Re: [PATCH net] bonding: fix vlan_features computing
Date: Tue, 6 May 2014 10:44:46 +0200 [thread overview]
Message-ID: <20140506084446.GA18366@unicorn.suse.cz> (raw)
In-Reply-To: <17712.1399362807@localhost.localdomain>
On Tue, May 06, 2014 at 12:53:27AM -0700, Jay Vosburgh wrote:
>
> The VLAN device doesn't get _HW_CSUM (aka _GEN_CSUM)? In
> ethtool, this is tx-checksum-ip-generic, I believe.
>
> I have only one machine with a _IP_CSUM only device (a tg3) set
> up at the moment, it's running a Fedora 20 3.13 kernel, and a vlan ->
> bond0 -> eth0 stack shows "generic" offload for the vlan and bond0, and
> "ipv4" offload for the eth0 itself.
>
> Is that not what you're seeing? Did this used to work correctly
> for you? Does ethtool -K permit you to enable checksum offload on the
> VLAN device?
In my case the problem is that while bond has HW_CSUM in vlan_features,
it has (only) IP_CSUM and IPV6_CSUM in features. As vlan starts with
features of its real_dev and masks them with real_dev's vlan_features,
the result is no checksum offloading.
However, I realized now that while I checked that vlan_features
computing hasn't changed since 3.0, I didn't do the same with features
so I better retest with mainline kernel.
But even if bond's features are set to HW_CSUM, I still don't think the
logic of bond_compute_features() is correct:
> >I thought the whole idea with bond_compute_features() is that you
> >could start with "everything" enabled, and as you add devices the
> >feature bits get chopped off based upon what each slave can do.
>
> For VLANs, it was originally the other way around; the features
> started with nothing, and the various slaves' offloads were added to the
> vlan_features.
My understanding is that there are two types of features: additive
(NETIF_F_ONE_FOR_ALL, support in one slave is sufficient) and
subtractive (NETIF_F_ALL_FOR_ALL, all slaves must support). That is how
netdev_increment_features works (except the additional checksumming
cleanup). For this to work, we should start with 0 for additive features
and 1 for subtractive ones.
The problem IMHO is that bond_compute_features() initializes the value
with BOND_VLAN_FEATURES which is a subset of NETIF_F_ONE_FOR_ALL.
Therefore these flags always stay on in vlan_features no matter what
slaves have (except cleaned up checksumming). But if this was intended,
we wouldn't need to cycle through slaves at all and could always set
vlan_features to BOND_VLAN_FEATURES (minus non-generic checksumming).
> So this patch may work for the case that the slaves are
> homogenous, but if one is _HW_CSUM (aka _GEN_CSUM) and one is only _IP
> or _IPV6, there may still be a problem because the _HW_CSUM slave will
> get its feature bit at the VLAN level, but the other one will not, and I
> thought that the mixed offload slaves case previously worked correctly.
I tried to go through this and I believe it is safe to advertise HW_CSUM
even if some slaves support only IP_CSUM (and possibly IPV6_CSUM). Even
if the vlan assumes the packet can be checksummed but it is sent away by
a slave not supporting that, the can_checksum_protocol() test in
dev_hard_start_xmit() would detect that and checksum would be calculated
in software. For IP(v6) packets, can_checksum_protocol() should return
true for all three devices and packet would be checksummed by the slave
it is sent by.
Michal Kubecek
next prev parent reply other threads:[~2014-05-06 8:44 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20140502165448.D8078E635B@unicorn.suse.cz>
2014-05-06 3:45 ` [PATCH net] bonding: fix vlan_features computing David Miller
2014-05-06 7:53 ` Jay Vosburgh
2014-05-06 8:44 ` Michal Kubecek [this message]
2014-05-06 13:03 ` Michal Kubecek
2014-05-07 18:08 ` David Miller
2014-05-20 6:29 ` [PATCH net-next v2 0/3] net: device features handling fixes Michal Kubecek
2014-05-20 6:29 ` [PATCH net-next v2 1/3] vlan: more careful checksum features handling Michal Kubecek
2014-05-20 6:29 ` [PATCH net-next v2 2/3] bonding: fix vlan_features computing Michal Kubecek
2014-05-20 6:29 ` [PATCH net-next v2 3/3] teaming: " Michal Kubecek
2014-05-22 19:07 ` [PATCH net-next v2 0/3] net: device features handling fixes David Miller
2014-05-02 16:57 [PATCH net] bonding: fix vlan_features computing Michal Kubecek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140506084446.GA18366@unicorn.suse.cz \
--to=mkubecek@suse$(echo .)cz \
--cc=andy@greyhouse$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=jay.vosburgh@canonical$(echo .)com \
--cc=mirq-linux@rere$(echo .)qmqm.pl \
--cc=netdev@vger$(echo .)kernel.org \
--cc=vfalico@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox