[GIT] Networking

On Wed, May 1, 2013 at 9:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I don't see what could be broken in that commit, and I'd *like* to
> just revert it on top of current -git, but that causes problems
> ("error: ‘NETIF_F_HW_VLAN_STAG_TX_BIT’ undeclared"), so I can't just
> do a straight revert to double-check with the current tree state. But
> the bisection was very straightforward, and as mentioned, I checked
> that boundary several times just because it looked so odd.

Ok, this is just f*cking odd.

So I first tried to revert commit 8ad227ff89a7 but leave the new
*_HW_VLAN_STAG_* bit definitions in place so that it would compile
without that error. That still resulted in a non-working network.

So then I start getting desperate, and say to myself "maybe the bit
positions matter". So do a full revert (so that those bits are no
longer enumerated), and then to make things compile for me I comment
out the uses I hit in my build:

        [NETIF_F_LLTX_BIT] =             "tx-lockless",

and guess what? I have working networking again.

So either this is some very odd heisenbug (but quite frankly, it
bisected perfectly, and reverting it *does* fix it with the above
addition), or the bit positions for those NETIF constants matter.

I think the positions of those bits matter, and adding
NETIF_F_HW_VLAN_STAG_*_BIT randomly in the middle broke things. That's
backed up by the fact that we have things like

      __UNUSED_NETIF_F_1

and

          /**/NETIF_F_GSO_SHIFT,          /* keep the order of SKB_GSO_* bits */
        NETIF_F_TSO_BIT                 /* ... TCPv4 segmentation */
                = NETIF_F_GSO_SHIFT,

in that array. There is some ordering, and there is some meaning to
the bit numbers, and adding the *_STAG_* bits in the middle broke some
subtle dependency.

That's as far as I'm going to be able to debug this. I've pinpointed
the commit, and I think I've pinpointed the approximate cause. Pls get
my networking going again without my disgusting local hack..

               Linus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 1 May 2013 21:55:38 -0700

> I think the positions of those bits matter, and adding
> NETIF_F_HW_VLAN_STAG_*_BIT randomly in the middle broke things. That's
> backed up by the fact that we have things like
> 
>       __UNUSED_NETIF_F_1
> 
> and
> 
>           /**/NETIF_F_GSO_SHIFT,          /* keep the order of SKB_GSO_* bits */
>         NETIF_F_TSO_BIT                 /* ... TCPv4 segmentation */
>                 = NETIF_F_GSO_SHIFT,
> 
> in that array. There is some ordering, and there is some meaning to
> the bit numbers, and adding the *_STAG_* bits in the middle broke some
> subtle dependency.

The other thing this does is it pushes some bits past bit 31.

netdev_features_t, which holds these masks, is 64-bit but we've
already seen one place in a driver where a 32-bit value was being
used.

I'll look more deeply into this, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, May 02, 2013 at 02:45:52AM -0400, David Miller wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Wed, 1 May 2013 21:55:38 -0700
> 
> > I think the positions of those bits matter, and adding
> > NETIF_F_HW_VLAN_STAG_*_BIT randomly in the middle broke things. That's
> > backed up by the fact that we have things like
> > 
> >       __UNUSED_NETIF_F_1
> > 
> > and
> > 
> >           /**/NETIF_F_GSO_SHIFT,          /* keep the order of SKB_GSO_* bits */
> >         NETIF_F_TSO_BIT                 /* ... TCPv4 segmentation */
> >                 = NETIF_F_GSO_SHIFT,
> > 
> > in that array. There is some ordering, and there is some meaning to
> > the bit numbers, and adding the *_STAG_* bits in the middle broke some
> > subtle dependency.
> 
> The other thing this does is it pushes some bits past bit 31.
> 
> netdev_features_t, which holds these masks, is 64-bit but we've
> already seen one place in a driver where a 32-bit value was being
> used.
> 
> I'll look more deeply into this, thanks.

I'll also have a look at this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 2 May 2013 09:03:37 +0200

> I'll also have a look at this.

By the mere existence of /sys/devices/${DEV_PATH}/net/${netdev_name}/flags
we have to preserve the bit layout.

So Linus was right.

So network manager is probably reading that flags sysfs file and
interpreting it.

I'll fix the layout to how it was before.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html