Date:	Wed, 19 Jan 2011 08:15:31 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	Matt Carlson <mcarlson@...adcom.com>
Cc:	Michael Leun <lkml20101129@...ton.leun.net>,
	Michael Chan <mchan@...adcom.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	David Miller <davem@...emloft.net>,
	Ben Greear <greearb@...delatech.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 2.6.36] vlan: Avoid hwaccel vlan packets when vid not used

On Fri, Jan 14, 2011 at 10:38 AM, Matt Carlson <mcarlson@...adcom.com> wrote:
>> >> > In the meantime, I think what we have should go upstream. ?Just to be
>> >> > absolutely clear though, your position is that VLAN tags should always
>> >> > be stripped?
>> >>
>> >> That's what the other converted drivers do by default (though most of
>> >> them also provide an ethtool set_flags() method to change this). ?It's
>> >> generally the most efficient and is now safe to do in all cases. ?It's
>> >> also the consistent with what was happening before, since stripping
>> >> was enabled when a vlan device was configured. ?So, yes, normally I
>> >> think stripping should be enabled.
>> >>
>> >> I assumed that disabling stripping in most situations was just an
>> >> oversight. ?Was there a reason why you feel it is better not to use
>> >> it?
>> >
>> > Actually, the tg3 driver was trying to disable VLAN tag stripping
>> > when possible. ?I believe this was primarily to support the raw packet
>> > interface.
>>
>> Hmm, is this really true?
>>
>>         if (!tp->vlgrp &&
>>             !(tp->tg3_flags & TG3_FLAG_ENABLE_ASF))
>>                 rx_mode |= RX_MODE_KEEP_VLAN_TAG;
>>
>> So if a vlan device is registered or ASF is enabled we will use
>> stripping.  That seems like it is using stripping in the common case
>> when vlans are used.
>
> Right.
>
>> Before 2.6.37 it was only possible to deliver stripped tags if a vlan
>> group was configured.  This means that if ASF was enabled and forced
>> stripping but no group was configured we would lose the tag.  In this
>> situation tg3 manually reinserts the tags so raw packet capture will
>> see them, as you say.
>
> Right.  VLAN tagged frames can still arrive if CONFIG_VLAN_8021Q or
> CONFIG_VLAN_8021Q_MODULE is not set.  That's why the driver was trying
> to keep them inline.  To eliminate the unnecessary overhead of
> reinjecting the VLAN tag.

OK, I understand now why you are saying that the driver was trying to
keep stripping disabled.  I was thinking about having vlan groups
configured as the common case for receiving vlans and therefore the
case to optimize.

>
>> However, in the current tree this limitation no
>> longer exists and it is always possible to deliver stripped tags to
>> the networking core, which should do the right thing in all
>> situations.
>
> Yes.  Even though the stack is capable of reinjecting the VLAN tag,
> doesn't it make sense to avoid that if we knew they would never be
> needed out-of-packet?

Maybe in theory but a relatively small optimization to the raw packet
interface doesn't seem terribly important to me.  After all of the
drivers have been converted to the new vlan interfaces I'll be
removing the ndo_vlan_rx_register() function from net_device_ops, so
the driver won't actually know how the vlan tag will be used.  The
impetus for this is that there were quite a few bugs and inconsistent
behavior in drivers around stripping logic.  Therefore, by moving all
of this into the networking core we can reduce code and get consistent
behavior, which is more important than a corner case optimization.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists