From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [patch net-next-2.6] net: vlan: make non-hw-accel rx path similar to hw-accel Date: Mon, 04 Apr 2011 12:51:51 -0700 Message-ID: References: <1301739966-7604-1-git-send-email-jpirko@redhat.com> <4D989100.1090207@gmail.com> <4D996B30.3080408@gmail.com> <20110404071453.GA2791@psychotron.redhat.com> <4D9A1530.1010102@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Jiri Pirko , Jesse Gross , netdev@vger.kernel.org, davem@davemloft.net, shemminger@linux-foundation.org, kaber@trash.net, fubar@us.ibm.com, eric.dumazet@gmail.com, andy@greyhouse.net, xiaosuo@gmail.com To: Nicolas de =?utf-8?Q?Peslo=C3=BCan?= Return-path: Received: from out01.mta.xmission.com ([166.70.13.231]:60249 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755426Ab1DDTv7 convert rfc822-to-8bit (ORCPT ); Mon, 4 Apr 2011 15:51:59 -0400 In-Reply-To: <4D9A1530.1010102@gmail.com> ("Nicolas de =?utf-8?Q?Peslo?= =?utf-8?Q?=C3=BCan=22's?= message of "Mon, 04 Apr 2011 21:00:00 +0200") Sender: netdev-owner@vger.kernel.org List-ID: Nicolas de Peslo=C3=BCan writes: > Le 04/04/2011 09:14, Jiri Pirko a =C3=A9crit : >> Mon, Apr 04, 2011 at 08:54:40AM CEST, nicolas.2p.debian@gmail.com wr= ote: >>> Le 03/04/2011 22:38, Jesse Gross a =C3=A9crit : > >>>> It would be nice to merge all of this together. One complication = is >>>> the interaction of bridging and vlan on the same device. Some peo= ple >>>> want to have a bridge for each vlan and a bridge for untagged pack= ets. >>>> On older kernels with vlan accelerated hardware this was possibl= e >>>> because vlan devices would get packets before bridging and on curr= ent >>>> kernels it is possible with ebtables rules. If we use rx_handler = for >>>> both I believe we would need to extend it some to allow multiple >>>> handlers. >>> >>> I totally agree. >> >> I do not. The reason I do vlan_untag early is so actually emulates >> hw acceleration. The reason is to make rx path of hwaccel an >> nonhwaccel similar. If you move vlan untag to rx_handler, this goal >> wouldn't be achieved. > > Need to think more about that point. > >>> Remember that Jiri's original proposal (last summer) was to have >>> several rx_handlers per net_device. I still think we need several o= f >>> them, because the network stack need to be generic and allow for an= y >>> complex stacking setup. The rx_handler framework may need to be >>> enhanced for that, but I think it is the right tool to do all those >>> per net_device specific features. >>> >>>>> This would also cause protocol handlers to receive the untouched = (tagged) >>>>> frame, if no setup required the frame to be untagged, which I thi= nk is the >>>>> right thing to do. >>>> >>>> At the very least we need to make sure that these packets are mark= ed >>>> as PACKET_OTHERHOST because protocol handlers don't pay attention = to >>>> the vlan field. >>> >>> Agreed. >>> >>>>>> @@ -3177,7 +3183,7 @@ ncls: >>>>>> ret =3D deliver_skb(skb, pt_prev, orig_de= v); >>>>>> pt_prev =3D NULL; >>>>>> } >>>>>> - if (vlan_hwaccel_do_receive(&skb)) { >>>>>> + if (vlan_do_receive(&skb)) { >>>>>> ret =3D __netif_receive_skb(skb); >>>>>> goto out; >>>>>> } else if (unlikely(!skb)) >>>>> >>>>> Why are you calling __netif_receive_skb here? Can't we simply got= o >>>>> another_round? >>>> >>>> This code (other than the name change) predates the >>>> another_round/rx_handler changes. >>> >>> Yes, you are right. Let's keep this for a possible follow-up patch, >>> to avoid skb reinjection when it is not strictly necessary. >> >> To do another round here was my attention do do in follow up patch (= I'm >> still figuring out how to move this effectively into rx_handlers) > > So you want to move vlan_do_receive into an rx_handler, but want unta= gging to > stay hard-coded at the beginning of __netif_receive_skb. I don't thin= k I > understand the rational behind that. __netif_receive_skb is actually late for untagging. eth_type_trans would be better but not path of control into __netif_receive_skb actually calls eth_type_trans. Eric