* Re: future developments of usbnet
From: Oliver Neukum @ 2011-05-09 8:42 UTC (permalink / raw)
To: Ming Lei; +Cc: netdev, linux-usb
In-Reply-To: <BANLkTimXmHr8aJ7WgRVvZj85t-gw68C_Ww@mail.gmail.com>
Am Montag, 9. Mai 2011, 05:26:04 schrieb Ming Lei:
Hi,
> 2011/5/7 Oliver Neukum <oliver@neukum.org>:
> > Hi,
> >
> > I'd like to get a feeling what people are working out there regarding usbnet.
> > So please, if you do something, or think something ought to be done, please
> > speak up now.
> >
> > IMHO usbnet needs better support for
> >
> > - batching protocols
> > - double buffering on the rx path
> >
> > with the latter having higher priority.
> >
> > Coments?
>
> Maybe another thing about rx should be considered too: we should introduce
> one flow control mechanism into rx path so that too much coming packets can
> not consume many of memory and cause out of memory for atomic allocation.
Do we really need to avoid it, or do we just need to recover?
If avoidance is needed, should we use NAPI?
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 1/2] linux-firmware: update firmware for RTL8111E
From: David Woodhouse @ 2011-05-09 8:23 UTC (permalink / raw)
To: Hayes Wang; +Cc: romieu, netdev
In-Reply-To: <1304928149-2201-1-git-send-email-hayeswang@realtek.com>
On Mon, 2011-05-09 at 16:02 +0800, Hayes Wang wrote:
> Updated firmware with stability fixes.
Applied.; thanks.
I'd be a lot happier if the WHENCE file contained version numbers. Does
the *driver* print a version string for the firmware after loading it?
--
dwmw2
^ permalink raw reply
* Re: [PATCH] veth: use batched device unregister
From: Eric Dumazet @ 2011-05-09 8:20 UTC (permalink / raw)
To: Michał Mirosław
Cc: David Miller, Alex Bligh, netdev, Jesse Gross, Paul E. McKenney,
Ben Greear
In-Reply-To: <BANLkTim2e1Gm_jqfY89is-FfRYTu2=G02w@mail.gmail.com>
Le lundi 09 mai 2011 à 08:56 +0200, Michał Mirosław a écrit :
> You could change dellink callers to always pass head != NULL. As a
> side effect, unregister_netdevice_queue() would do just what its name
> suggests.
Good idea. At first glance, macvlan and rtnetlink.c would need a
change.
This would help macvlan_device_event( event=NETDEV_UNREGISTER) use batch
as well.
And yes, unregister_netdevice_queue(dev, head) would only make a
list_move_tail(&dev->unreg_list, head);
Will submit a patch soon, thanks !
^ permalink raw reply
* [PATCH 1/2] linux-firmware: update firmware for RTL8111E
From: Hayes Wang @ 2011-05-09 8:02 UTC (permalink / raw)
To: dwmw2; +Cc: romieu, netdev, Hayes Wang
Updated firmware with stability fixes.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
rtl_nic/rtl8168e-1.fw | Bin 6236 -> 5500 bytes
rtl_nic/rtl8168e-2.fw | Bin 3568 -> 3920 bytes
2 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/rtl_nic/rtl8168e-1.fw b/rtl_nic/rtl8168e-1.fw
index 33104be92d21aecbe9ce5928d909491646d016ce..d203bd5d0dc10580fa7c4b9b0df2792e6ab8f888 100644
GIT binary patch
delta 1684
zcmZvcL1<i66o%h>lR0LQF_~IYO(99ACe~?FqJ#z;4OFY*LR&#$l5P|VQWwQV_j8&=
zkSTOC&_#=3a4DJ5RS>~6wHibcZQ4blh`gCh)7Zj{ZqlYsGJf~H`%Eh?-n;id_uT)S
z^WSstZ-b5W*ZUIE<B58o=MN^*-jkF0@kD?3f{LcG7XK7Y_KEhbi=M0EbzZc8B0rr-
zW&H=DljlUAEsFX_MN>XwnJs%oM^d6WaA8g~?uhP#s`NYfRj_OEU*N-F*3O!B!W3Qv
z$3&ku_#!wBZn3xoPJ!bA&T8o<hAs?Z9kxVkJ4EMmj0C-6#|1miF_y45_#QxN>hnL^
zPCP;48Mq20;ITG9K)1ji=7j;#x7k<mMaztCCA(eu_Ymh0$Nxd}s-4$<6g2^T$c##T
zs8plcAPFk)heUs2Jw_0tRQXp?!xMiCd>d@JA{_B~?43=E_GV|ul4>uV1u(C*{eQ&`
zvcE>P9$6RhZ69O)l4#49RB??t2{?=!_*5Q>Uav5&fNzN2A7{QQdgr?64eUPO$^n!i
z;p~YG(QYQIgQ5o*{W~}#(_%!hL@)@Be{4w>MVAZ-zU7xiUtv612XERv^iGC7<L&U$
zc2&>XQDYN(u+Ikf$0VygCq$p>W}zy2J6eD&sKAg>!T5d1T0^qH4p|-h)*C2*?*>g=
zr)vMyb{}Nu10(-wj;*5yp6Rgni>L8Pi9R<YI`cl~@AZpzA)hSM@t48has(*wM1;Tl
z51tS4AbVnw&(+Z8M0jBvPZ-XhL);#%+Zb*(GT0z)gl&)uNW006XlZc1(V7jG<h=JI
z@7Ui|9}``6>2QZ&eE;&;M3U_7EsD0`Urge&M|4LU=l^JSKT%Qj6Bbcqd4lUjmqxN~
zRXBf+qu4|()G7OX?}#45Hh`4tDfW@X2w#lj622)@`BBk5_$T3hY!As?Zk7FdYzhLh
zS|_8raHbc+Y9Df@r>K6CHScxNFDCOBJG;D*sr+nbw*=H9`CFZH*<d;v>iei%rYU@+
z`PAN9pA4hdt+H425Eb|<G}o|bf=0=>G#SBGdbuh&(HBgEd?4C-Sag(m301!0p&2IO
zVY2r>6kW^F!PSmnqB*44{fFo@*t|B&?^2>8dc;;(yAt}D*Q@v%9eBt-8Z{~>;2Ek7
z9+A7E_YB@GdXTQI?i9TXHV;bu9)ZvBP8e+B)iVTqlYrc}+B2b}c{UA!2{ePfjfcWC
zM4;4>P^lb2oi#{SMVs9D2G_`G5gn><&Woa-Ir-ULqeDv*M!>i_y(VT{zu|b-7)_5g
Yo37(JPKV>T^UWszVD8N4Tc3XKA2%C>vj6}9
delta 2448
zcmZ{meQ1?+6vv<Y+}-0Mr*k^PnRj>dZB94L1YarxF(INCWLBI1B$Q<@68_a6+C7__
zl!H(%2=#}oK!S1aRS^6`Xme>6GILj{=mmRry_=-5YY<Ltx<2Q5o(oF|_V7FB{LcBF
z_ut(_<8LdEH&lqa{GCfRS2Vg!U8%N;CGn9C(MF`1o1(K8i!Qz;+Fd~F6VYcnQ%5T5
zlXq^5dUr+JyG56zq00lh4mykJ*gK-vJx)jT24|jg52trk^ypq-XNg{_7X9%x(H%QP
zr(tvHUHBf+-rj(B;TI%CH)YW|ihK0dLiZk(q`D}rg7L;g%dQ8f+~CZMUfm<Q*PQVU
zuN7U#*_<JtI?>rrixx~iN3@!p^euSKk&sUiNOpl}`e7Vy5EXp1BXc*4zLF5VcOQQf
zNx-RqpFO~CRP;+Oy8+I$i#~)SqtnfzQRD_Nv0e0bStv&*Mq%sC84k>TH?pz`QLAi`
z%HG{6x*v=l?B{Ag4}<$S6|YRrC$b}=3jsKGNOVgF3RI#U<$^`r11K4Nx{&VmLl279
zH3d2jAg0@aZpSekY@w27?@<-*R+)qU!8coHNHp1j<H}Yd#*ueNw01ui&|}j?6VS)@
ziGI*7x(Gct5(KiF!1ke2`<&>|R?&W{QH}1w8qt>NqVb<ak3R<9-&BTRykV+I0@Itp
zD2i6`ZJI&SMf%?YQHP)~DtfI^bU8gN<mr8b=rZWUd!nO`XoJbG;cS`cc}m_yKx1TK
zL5_8TVZowj!FCdyH!qNN<9z;V(Ib^O7tuomV%2h~R_!ACZzb$Xh?ZwLF|?_-K5kq@
z{}__{hw1sfpyzj~&`yHQ(brmK8+HWSVcXDlgUspqC|I^J-iYXBWG7<ifm7O=3|kXj
zBRa`hk(?BL^i{wq<7R-Z9mQ|?-oR;#=*l!Jwp{e1&&m9}=nS%~L~j;lK18|a{etbU
zfo<Gu9|`Rk=Hg7zk6%XT3IY5cw01u;R5Mrf%0cc4Dp|@dUB<316<veF8hW3_d79mx
zMx+jvX~CJ@4RQBFgy;#}MQ%e>+J?+N`byROb*-nrRH2?gyeCBa4g_=MQjIILb|bct
z9@~Eee<4_p!0q9K-vp4o3DJ=MjGi~b+mPCQ7QP-n7V<yA*T752--oY)KQM;V)8RP~
zp69}|&k{=pyXw-nHyOUEkxK;u&>9QMW}>GFn#`L3whi%~=gd>uVWu0oWq!b8NWT_6
zOSV3JM)Vt_2_%X`|6L)u^@{^pEQxTnMT%kedqAV27tH|BX8{@^TOZ7Lz>F^e#;~>5
zJ$;gF6Rf1sr&u#+Kl6d`3^R7ow};7EX1c9|^nV>g(hD<0|FdK<1YPFvYBA~gJZpG~
zDdud0A06jyht6N)yDYkbpl$N+P#JF<!y4xtWsO%*CGLECoLmU)-^SPIOQM4zhIfJf
zKWQLjSA2>$`E`uoheM;8l>cTMWd}H4Ap216v5vlT>{@ZY4V!o^rJo(jN5DevV}%Kf
zgH=8i%a_xCTRZzPuqiI>_tZH!i=jOtwY!2xqyWFGJDBWrz35i9#(p_%MsjRPzlfe<
z;_ZRT4ZyQ8Up>H|4*6^nh1^P%==+(eN{;Bc2lziar+`w*|2T;}-~_(pq==8wn&(Ke
zAMBE^<PeuGd;SlPOth7qq#8~xnD87K^aC_CkPNzRB6xu6G7OvPREd^?%Yyg*cT|LS
zb#fQGNmXwLucqG)Dm|Ge5p2C0`v1Z({pL%*NJ;5g^cEk6<77WhRjiz6yMoNKbr=yq
z$tHtMIW}GW1a&d^WfbfWlf*>WQu_&ZcZzNY!*46uM?BG`1dyvO4>IiQN_|)r7aylD
y^+i=*+$YIp0C<B7{#{_CFdtl<Iyq;<oImNu<YdH&I8LSGL{7&XM^g>eE&l>hAD*ZH
diff --git a/rtl_nic/rtl8168e-2.fw b/rtl_nic/rtl8168e-2.fw
index 7e7d7c220a686dfc31d0dd83b69b55998623e8de..7ea5984cff6b238f2a37907a8396762727f5692a 100644
GIT binary patch
literal 3920
zcmZ9PYiv~45y#K&ddJ2XFvJfoZtS&r6Y~tOP?DIIBu+yTLjna<rAZ$YR8^`fzElD+
zJVQ;3QYlSUC6IL`8im*<>W8W+P3<&E)0b31)2hmc>aMX3F|ec$8ABXze{=6ya4PN9
ze`e0S&YYQZ_d3_0Zo1P_aHgx*+C*(cH$%I&I(5iP*42RS+X?5wLZRLjQZ7^BlG!Tf
zo^vj}ByB!!t(Urtv)}FYLZK)t&sv-g^Ichs4Ns<47sG%2;&hGulp=44=;1<I)rl4g
z&h0ChPuH7dtxzcQd|9vQ7IT+Wjh6CEcGX($0<8qg!0BM3HPFCtXP`5{jvawEfR6Qy
z$3~A$9%p)-<#9IH@wGtbfI}^T&IMBs1Uk>wUk-FWxb$a%E&ywP9OzwO^O8WD!6fk)
zf*))RbP+fS`^DhV^?@z{-N`_gg1`T2pv%Dh#JSt#<hR`83Xdy2uJX9r<2@eNc>FAQ
z34d#i-y7(?;QQMGy$>8g-U8Zw)`9;d&-LIG@_Ybv)agM`snbJX<(@!WLCf!9TPLqC
zcznd;29J+=e9YtHpw+7lw0dm>|41D+f#&ZC@E_olV3v5BL96E$L-OhX*MqNt8SFa2
zlBWXg0xNzI=o!$?`{&>r=)D1!QP*FA6WPyM(4qfp@XO@!CfLgQZ@>(E54aP)7fiks
z=v!cxIKKtc_<tMxYj2?MfP=(&7rgR8puYoe!oLUJI1%WyM71_j&pW^}>Qx1P#J;M*
z6me?6>BOl8-HY@k_ze01m;+~+9Di+~V!zR^ZvxH!39yCwJqcQUHX9Omi^<XdB53iq
zf@Z%B?10}6j!~D3$=Lt=6v+Ak(Cpp>pThqiy!?Vkn<vL(b6K24r_5wh^m?mk$ZTFO
zh-y}}8(DH#)Pc61{J!WcLwlBs?kmcFDtcB$kA<SSqoUtCCYq>+&xzKzBTGP!m*K}`
zKNv4NE}BL59`Vn1i?-*9I~ZdM+W22yi!u2j{@^o*V@y0Ny8nIA{-vVN<MY^nXwQ#C
zk3zdEqOS&`&q3#J2zv&JVS5aDTF5cSbA26ai}5kf%kWWxEF6jVlj*{4QcNeYzj{El
zAxRz3nTActXsl~{igYXE{h98GJIK#;|04R62GP0DqeG%2CFB~4b{!Y3%3w#WkM*Mm
z@Aiw<nazJiUuyRD-w|DVNVJJuhNqHiljw2cHJjdpqVFycoo_V0et4JY?4sW6STAL3
z+eOz9cZSj1$OApg{U-a|MSVl^$vMssoQwTA^6g28UMB8hY{EA3WG&2*ccbX!47r{Y
zJ=BB0%c5=6@Lr?u5q*7;=nME99fK$43idRT7QJ>8`(2`AeWHElr$9aniHqOdW1^2E
zTiD4So3KlXuH;Ew7Zsi;&fj9+Q@j7qhJ1U<@J+6Z$aVO2VhM3xi1&DdJr-j>OWo|8
z&Tl~9u?KQVY$aX=a(dV5uoZbd=NM+l0Y1fgaV<w~Gm(W})(+i3zn&!SAZvs4{WAP_
zRf$ezR+7}g>`PBl2jtty^W|RZOI^AXAa*@Y^k2Tt*Qm4QJ^D%9CpYO`>y=JwIC;)(
zx~$RD%s_IM=r^fJm}fuqSA?8N*e7~nkLX_Pr=Ak6rgxq`FS_IJ*z6F!QOfMWpEmw<
z%pV}1?#-g7)}x1x-36ysqr<w<`DLO*_{)?3Ww1{~_kiIAp1q>0jpiJ?=!3>ZqW81s
zkU6O8r*Dz(tOnKd%vutD^8wL`<bNM$AKg9p+=t9^8GH41E$m(i@wuNfoP*yaJ@p^z
zFpvI-=4roZ8omj;>m%sS7q#=A<#qe<xxjRZJBpuMbBJAP4*6mia%Seg(ej=^KS5{6
z%lh$=gFX&5pFqu{bHPuFm<O>PUE+P_KAmUCnV&!G=h9+uZ`_JeE?RzsnHwheEWK;%
z<MKA!tL<^m+S<T^I49Q;=QNerHpkJudDY{UaeE`zHRKx2_fULhBlKf}*$p|90WXWz
zeXqDLao;eT^-lCNuX)=IqK|U6Z($qRc|NksGheK&-R0LjU%VTOXAn-KZ_u;;*v4G+
zzwLR>BM)!;98KKL-FiMfJFaavIoTQ1z?<LVOy4Bt5qxILh5v*xotr3nH|JJahc0tx
zy)E6Wkyiq}L1=rQbu;f-=Cq2K#`n+z-{P@5B+nf(k#n(khMmtDWGfEgD~oIse*Ib+
zw0p#Ktv~CSkCWUTr%U|2{3%|u_ko?mkv?XUHM9FVdyaVFet_RXzLUvyi^&H?FM7G@
z4AP5b=tO&kF2$eiC&hjao*}QVaM$BM+SeTR)k+UjFVoM_Lldw~;J<=;q=^yfoxtv5
zY}e=OQ-VM9>!_jm&EmJ3d?P=|mixM<@sq-j>E!7ft5J6v{R(`7rTCkfWQ|;HZH#>F
zo$$%n?ehyi7CnqVdtZf3%q_WEZ}iQK?_Il7wyzSM^bU72XH<{B3mNLtF1oanJMn31
zvc}&V?D@<$xK9s>t}=hmh+aakpR+J*x3l>$J{!AZ&TWkE-8}mCe#^JTdfnvN^?BxT
zB{t-fqZZe&-%aijKg0|fPc0WwgMRvF$ovnWcbT}m@nyWl!{+2e)MW~?Al9>fGJA{D
ziM)b)(q=JwzY)vcqtPs10tfkC;(Y8~@Cs)zK=0pKPCbb=&`hru={uQIPrtH|v*Z3=
z-bWpeivF26_KqtQ5-#CfrE`g1`v!|?*K6Ng_I$UoRukave^*A|V4t(^x+(m(<SE<D
zf5*u&oiJHTjV9oYP8$DwjV9rZPVtz23f|}v<IyjHH#*H@`e}Hhg(p>3qXKWVo5aI^
z+$fiiDx=&UCbv(2mpaEnhCisczFUjmjWvApj{lA<6l$Gov3=L}X4q4qP-nLAb-nB0
SE$?}eJTRxogWk9%&ixM;Tn66&
delta 1897
zcmZ9NPfQ$j6vt<VoyVUd0j0(Qp+JEawAA2gs#T1M8WZiGcrZ{sc!5}rM-vZi_O)f{
zVUs3G(t}aNLt}#MLgK|6HO7-M5n?!+*=0*(o9vosN+DbD^X9j!)x+ey`MrPN_kHg-
z^Go+1oy!x6u8&3g1JS^qq-YAMre78PdiJBl>6Ua!)bm6y{wiARnB7Ud@2}5JhDras
z+0Vj`R9lZ|&t1_>(FOgkqMerDFVWARAPhbGwrE?QXx{^VuVNcEzaNPo5`EnKKEZE5
zbl?HMvvr~g^LtKoqQm)BDx&uiqU9Y)X><;Jk!X}BfiHUSu&BQ(I=w>B3DGmhuwl;w
zv=`Zv6<uu+UGYRuOp6{P`N|p5XI~H<K`$?h?p~uPMGSn{ExL10^julAV0m^$_X?s%
z$rE*?L{CvT&$D=%4v&h~fxeEwt!H>Zzan%*AxoYkdDQlAOqQ4iVq&mQiY||c3h~z{
zR-uq}xwGwxXDFV%BKk%T4X-&c<0-mmrt5B!M`pmOb(}6iZC#H8q8kuq>*L*68b8FD
z>703KSUQMpPBgtq0Z+7jR5VF<Wq^*8caHdMJwTrpy>S+1zH|%+Irhb?QT>vm+K}G|
zxzV=`ZP5-iW<&=U(V^Y+Ueg;yub}s%=O|M8K=d}CN&vWnEFjM#qYcsP7uXBHtnC+F
ze^+#BO0=GswQc6VJ}<hk;0^+)8oMO=GRUT`(;1G((HBg|XB0gu0q(YwatB!u-8e6L
z!HV(ZkFyKN5g5q!iazopdvE9MqWg>7j`w&+-i+Uw|0R&*#+d&;iXep|b{63HDPU@B
z<Y8lkj^jCAWU&Te0>7<4DSV{L3`K*Gx%Y_<ZB>1ldD!oAZv9N*VVB1kj#4Cz>9s)S
zlmv0DKx0*39AP1QL@!e;N>*vdKWcP|Y#bJQ;IbRr|A96DGz|k;mM$0&J;H+4YNVPd
zGK2AMlj!_mhkmH$0P+Tq$HANhfEYI0P0_Cml;;?zFxygs9BDWI0GYrR(OkSE2A2D8
z`w=fNV_U^m^qGHrNWzH2cwzFQ@31g?B=DXW{iYjR7uTj1e3HQLtM3-ad#P%(J6<Fv
ze#chf-L7EVEzLYkGx3hFLuF!qm|*@<k(pEv>|^QEVLY4aXa<blQ1lh(t;_K(U<{Q<
z|9iLXmNd-(c%FO*ME9X*Xr%Kgu3sw)42bxai1W+&<lN*Mr$E$BQlG$pjU42Ca0UHC
z_c56PkjG-03>eY$5@UjqHvtjbNE}N5+8Fcrj%N6Vbkf8eF$<zUk-wJ5{uKpAkpSS#
z2KT>AU}2K2uO4*PgZ3@xX%XE-#^<;2;k?m&Ky7#5bnCXnxplt80=^DI@z3b0YveTw
z*BK+d?2*M9BP{J4-7ho)E@b{+(D5xCEigx5so)aYLK%%qo=cptM0`O0p_ZEvQ%eI@
ze3IhUWe7{n<O}gP$^!#_$k{w><z4rGsNCldnV9|Up|QtiGNJmE+1v9y-}72M&%Y6R
Lo_?Eb>GJ;u^9HdZ
--
1.7.3.2
^ permalink raw reply
* [PATCH 2/2] linux-firmware: add new firmware for RTL8168E-VL
From: Hayes Wang @ 2011-05-09 8:02 UTC (permalink / raw)
To: dwmw2; +Cc: romieu, netdev, Hayes Wang
In-Reply-To: <1304928149-2201-1-git-send-email-hayeswang@realtek.com>
Add firmware: rtl_nic/rtl8168e-3.fw
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
WHENCE | 1 +
rtl_nic/rtl8168e-3.fw | Bin 0 -> 2804 bytes
2 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 rtl_nic/rtl8168e-3.fw
diff --git a/WHENCE b/WHENCE
index 60c189c..d65a04b 100644
--- a/WHENCE
+++ b/WHENCE
@@ -1581,6 +1581,7 @@ File: rtl_nic/rtl8168d-2.fw
File: rtl_nic/rtl8105e-1.fw
File: rtl_nic/rtl8168e-1.fw
File: rtl_nic/rtl8168e-2.fw
+File: rtl_nic/rtl8168e-3.fw
Licence:
* Copyright © 2011, Realtek Semiconductor Corporation
diff --git a/rtl_nic/rtl8168e-3.fw b/rtl_nic/rtl8168e-3.fw
new file mode 100644
index 0000000000000000000000000000000000000000..cb494071d0273c7b3102b97552f4bc7b756c0e17
GIT binary patch
literal 2804
zcmaKuS!|S56vyvuXDEwdX=z8WolZ+xrAP=!Vq#-Nc`(M9m<Srf13vJiQ4@-BIfa&`
zMS>d`6R?UeXp0DaGtxc~Ow<@#AOWSxH%JAQw21-A(sulv@B5~F;_~q2-gE9*{^x(s
zJu}W#sjG16T$w8jxh!jsTkSNBzTQ=$cQ3}A^PHP)Mp<?7_Vv!~b}qHVg}!E6^Ecaa
zhfmgFTN>J?u(?)9sXFmvo4q7vYMSp0SElP4GJ2TRHD>fnTi?j&5!S;mW%Q`YZ)UX1
z<d-uVi<N20Wi-K>U`?@(F3G5{4)4oon)L)SwfH$4IpT&j{MJ>M`xqL)kaQJVo32nd
zmeFOlj%KvBf^`JhR%G8V;^Xi!U9={r)D{;VUM=cT<8VP#+eHtdi(eBhi;2c;pA)Sw
z?jICg!`}SjJ4FwwXqzXR-!8hN4f{&uyl7=BI_%n_I_ypNR#dl3v>n|y<p1iBXwxat
z3;RXi!KQ6MwD(=n?u6*u&7v#F*;|9m`d%mxB1?<tAT|?NyVr?s$Jbp3TSjy@`@tK+
zJ+hL+YU~}uj{Wv%Ov4(=Nn(1}1Dx4T{7QqpakQ4Y5{;rIU`!bYKZ`!FS#&A;k^4li
zCyC>Uc2d{fS<x&!wq3vud0vy~Li71fbpE}>v3-~5`}i!j{kNhoMEoaJi&jVcmvJ@=
zJMWU{F6zpG?OF=Ha)DgTI|2MP5x?2sDxRfkMQg(|4@?$kPBG4$JL1%d)<xo61@m~E
zxuSC;XRpG4azyk7ybaw1;|9@f#5Vr=o)7r`tzL9F_$%@2rnal0JQ=xiTA;fg)lCm{
zx1zckfo^YA5X<-Fek|Uu=7?@47|`wHzT(*CxUbe<xfgn{6h4-KYlxgrgJ=I_^lAEM
zIr_EK@ezI8iy!^ySnIpl{|TonME4Adj?zEHT7RHctM?}L#^~Lyr@+Gbhar|uYI}5^
zXtTxG%YBh!UWe$9&mhxJ)tmW{JGe)CXOEN9!}i#5aBd|}pJ?U`-&xUfVC<)-(|kL2
zh^_}q`Z>`l1<_~W*uniHmDrx+>lZzL1Ra>VW1@S(WAEn>y>aXw(c|>!R`T@0-I=85
z*)VR=_XxR8UBMrWMeJ7p5Vei%;X6UUk8<DO_Rfp`^1J9C#7x6q0zR|g@ZkGmsP{O_
z(;uU(N%I4%^-@O@@1*YbzLY!j^(~@zWpD4Tr+xi4c&g>S!PA97V$BX$YVvWsCi*&Y
z{e1U}u4KQMyY8F?Pb-6at+u;39o*+;<AfR0F$o>^LR{0uPx98b1U1^5Zp{-DKAPz1
zI^xBt`_mkEIUL~Xq>dEWYw#UDi0ypQmazZL_H%e%P_(7C6!lCu+l$yP4Bkp@7<VIi
z7h&5vKz?$L&lGYb;Hnxwk2AwkjgMC%dWAmE(ns-x=m*3$t~$w60*9^aJBX72-wgA;
z6=180_~t_2o4h&S7oJPtdEA$nTksu$-+$x7n?Nst+n*(tgPtG9H&JwAb1*Mb?Ysdy
zkMLu@m%wke_-6|u_&Cc_XQJpA;mZ6RH5ShX76m-GkD2kD*^EC;&DQrL;2-23%|40V
z=DW=&<J_ZHP4CEIcXfa|uF?mwP(F%02{|6hUn3`wE$@g6_^=re-@u#UtYr{i^foI-
zcrV76=^SsW*|5;FzRu%K2ycemu_}6Rzvuun$>b#Qi*bsx#&X;uM+^NsM4Y}&<Redx
zFHQ~qoVpzJ^$hEAW|qyHRZ~Q()-VIWbF!cJ1~>im{8Vz-`~JVLoPJ1tB3k(VpR;cq
z549yM4sWVk$qW8X<XXk|Uzp~?dm}aE3&HPpjQ6?tvjF3!v&>}THF4M7;BBXOiog9s
zF>vEIZ8AI_W~DZNKJ5$Yws{?ujYFHW#^(WkJG!`Y<!z7G1&-)U&tto>L9`)HUyyrg
zPV@=(`B&iJVbPyF^k9Cnhkmj>Slwfy59L$Fqy2}R%G%R#hg|&c<--4_g@POZZ(97n
VdHX+Uxf?6kf6?+D|9>1#e*sYqwB!H)
literal 0
HcmV?d00001
--
1.7.3.2
^ permalink raw reply related
* [PATCH v2 net-next-2.6] veth: use batched device unregister
From: Eric Dumazet @ 2011-05-09 7:45 UTC (permalink / raw)
To: David Miller
Cc: Alex Bligh, netdev, Jesse Gross, Paul E. McKenney, Ben Greear
In-Reply-To: <1304916275.3207.79.camel@edumazet-laptop>
veth devices dont use the batched device unregisters yet.
Since veth are a pair of devices, it makes sense to use a batch of two
unregisters, this roughly divide dismantle time by two.
Reported-by: Alex Bligh <alex@alex.org.uk>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jesse Gross <jesse@nicira.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ben Greear <greearb@candelatech.com>
---
v2: added a list_del(&list) for safety (see commit ceaaec98)
drivers/net/veth.c | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 3b0151a..b41d6a9 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -416,8 +416,17 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
priv = netdev_priv(dev);
peer = priv->peer;
- unregister_netdevice_queue(dev, head);
- unregister_netdevice_queue(peer, head);
+ if (head == NULL) {
+ LIST_HEAD(list);
+ /* make a batch of two devices to speedup unregister */
+ unregister_netdevice_queue(dev, &list);
+ unregister_netdevice_queue(peer, &list);
+ unregister_netdevice_many(&list);
+ list_del(&list);
+ } else {
+ unregister_netdevice_queue(dev, head);
+ unregister_netdevice_queue(peer, head);
+ }
}
static const struct nla_policy veth_policy[VETH_INFO_MAX + 1];
^ permalink raw reply related
* Re: [PATCH net-next-2.6] sfc: Implement NETIF_F_LOOPBACK
From: Michał Mirosław @ 2011-05-09 7:36 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, linux-net-drivers, Mahesh Bandewar
In-Reply-To: <1304640751.2857.34.camel@bwh-desktop>
On Fri, May 06, 2011 at 01:12:31AM +0100, Ben Hutchings wrote:
> We already have comprehensive support for loopback testing, so pick
> the nearest mode and run with it.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> David, this depends on Mahesh's patch to define NETIF_F_LOOPBACK
> <http://patchwork.ozlabs.org/patch/94190/>. Although you've marked that
> as RFC, I think it is ready to apply.
>
> Ben.
>
> drivers/net/sfc/efx.c | 24 ++++++++++++++++++++++--
> 1 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> index 38a55e9..b7bfb49 100644
> --- a/drivers/net/sfc/efx.c
> +++ b/drivers/net/sfc/efx.c
> @@ -1884,6 +1884,25 @@ static int efx_set_features(struct net_device *net_dev, u32 data)
> if (net_dev->features & ~data & NETIF_F_NTUPLE)
> efx_filter_clear_rx(efx, EFX_FILTER_PRI_MANUAL);
>
> + /* Toggle loopback if required */
> + if ((net_dev->features ^ data) & NETIF_F_LOOPBACK) {
> + enum efx_loopback_mode old_mode;
> + int rc;
> +
> + mutex_lock(&efx->mac_lock);
> + old_mode = efx->loopback_mode;
> + if (data & NETIF_F_LOOPBACK)
> + efx->loopback_mode = __ffs(efx->loopback_modes);
> + else
> + efx->loopback_mode = LOOPBACK_NONE;
> + rc = __efx_reconfigure_port(efx);
> + if (rc)
> + efx->loopback_mode = old_mode;
> + mutex_unlock(&efx->mac_lock);
> + if (rc)
> + return rc;
> + }
> +
> return 0;
> }
If other features than NETIF_F_LOOPBACK were changed correctly, this should
update dev->features of what changed before error return. If not, device's
state will diverge to what is in dev->features.
> @@ -2472,8 +2491,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
> net_dev->vlan_features |= (NETIF_F_ALL_CSUM | NETIF_F_SG |
> NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |
> NETIF_F_RXCSUM);
> - /* All offloads can be toggled */
> - net_dev->hw_features = net_dev->features & ~NETIF_F_HIGHDMA;
> + /* All offloads can be toggled, and so can loopback */
> + net_dev->hw_features = ((net_dev->features & ~NETIF_F_HIGHDMA) |
> + NETIF_F_LOOPBACK);
> efx = netdev_priv(net_dev);
> pci_set_drvdata(pci_dev, efx);
> SET_NETDEV_DEV(net_dev, &pci_dev->dev);
You can remove the '& ~NETIF_F_HIGHDMA' part, as it's now allowed to be
changed (commit fa2bd7ff9247f4218dfc907db14d000cd7edd862).
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Scalability of interface creation and deletion
From: Paul E. McKenney @ 2011-05-09 7:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Alex Bligh, netdev, Jesse Gross
In-Reply-To: <1304888447.3207.66.camel@edumazet-laptop>
On Sun, May 08, 2011 at 11:00:47PM +0200, Eric Dumazet wrote:
> Le dimanche 08 mai 2011 à 08:48 -0700, Paul E. McKenney a écrit :
> > On Sun, May 08, 2011 at 04:17:42PM +0100, Alex Bligh wrote:
> > >
> > > If 6 jiffies per call to ensure cpus are idle is a fact of life,
> > > then the question goes back to why interface removal is waiting
> > > for rcu readers to be released synchronously, as opposed to
> > > doing the update bits synchronously, then doing the reclaim
> > > element (freeing the memory) afterwards using call_rcu.
> >
> > This would speed things up considerably, assuming that there is no
> > other reason to block for an RCU grace period.
>
> Thats not so simple... Things are modular and better be safe than crash,
> on a very rare event (device dismantles are not the thing we expect to
> do very often. Only special needs might need to perform hundred of them
> per minute...)
I was afraid of that, but had to ask...
> For example, in the VLAN dismantle phase (ip link del eth0.103)
> we have 3 calls to synchronize_rcu() and one call to rcu_barrier()
>
> [ the 'extra' synchronize_rcu() call comes from unregister_vlan_dev() ]
>
> Maybe with new VLAN model, we could now remove this synchronize_net()
> call from vlan code. Jesse what do you think ?
> Once vlan_group_set_device(grp, vlan_id, NULL) had been called, why
> should we respect one rcu grace period at all, given dev is queued to
> unregister_netdevice_queue() [ which has its own couples of
> synchronize_net() / rcu_barrier() ]
>
>
> The real scalability problem of device dismantles comes from the fact
> that all these waits are done under RTNL mutex. This is the real killer
> because you cannot use your eight cpus, even if you are willing to.
>
> We can probably speed things, but we should consider the following user
> actions :
>
> ip link add link eth0 vlan103 type vlan id 103
> ip link del vlan103
> ip link add link eth1 vlan103 type vlan id 103
>
> The "link del" command should return to user only if the minimum things
> had been done, to make sure the following "link add" wont fail
> mysteriously.
Hmmm... One approach would be to use synchronize_rcu_expedited(), though
that is a bit of a big hammer.
Thanx, Paul
^ permalink raw reply
* Re: ath5k regression associating with APs in 2.6.38
From: Seth Forshee @ 2011-05-09 7:02 UTC (permalink / raw)
To: Nick Kossifidis
Cc: John W. Linville, Jiri Slaby, Luis R. Rodriguez, Bob Copeland,
linux-wireless, ath5k-devel, netdev, linux-kernel
In-Reply-To: <BANLkTinkiTQU2k7vBEc0JPGa01AUVCvp2Q@mail.gmail.com>
On Thu, May 05, 2011 at 05:30:42PM +0300, Nick Kossifidis wrote:
> Hmm I don't see any errors from reset/phy code, can you disable
> Network Manager/wpa-supplicant and test connection on an open network
> using iw ? It 'll give us a better picture...
>
> If iw doesn't return any scan results we are probably hitting a PHY/RF
> error specific to your device (not all vendors follow the reference
> design). Maybe we should follow a blacklist/whitelist approach for
> this feature.
I got the results back from my tester. He was able to get scan results,
but it took multiple tries and the direct probe failures appear in the
log. He didn't enable ATH5K_DEBUG_RESET this time; let me know if you
need that and I'll request he retest with the extra debug logs enabled.
[ 25.070218] ath5k 0000:06:02.0: PCI INT A -> GSI 22 (level, low) -> IRQ 22
[ 25.070290] ath5k 0000:06:02.0: registered as 'phy0'
[ 25.830368] ath: EEPROM regdomain: 0x63
[ 25.830372] ath: EEPROM indicates we should expect a direct regpair map
[ 25.830376] ath: Country alpha2 being used: 00
[ 25.830378] ath: Regpair used: 0x63
[ 25.830382] cfg80211: Updating information on frequency 2412 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830386] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830388] cfg80211: Updating information on frequency 2417 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830392] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830394] cfg80211: Updating information on frequency 2422 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830397] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830400] cfg80211: Updating information on frequency 2427 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830403] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830405] cfg80211: Updating information on frequency 2432 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830409] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830411] cfg80211: Updating information on frequency 2437 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830414] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830417] cfg80211: Updating information on frequency 2442 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830420] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830423] cfg80211: Updating information on frequency 2447 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830426] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830428] cfg80211: Updating information on frequency 2452 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830431] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830434] cfg80211: Updating information on frequency 2457 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830437] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830440] cfg80211: Updating information on frequency 2462 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830443] cfg80211: 2402000 KHz - 2472000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830445] cfg80211: Updating information on frequency 2467 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830449] cfg80211: 2457000 KHz - 2482000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830451] cfg80211: Updating information on frequency 2472 MHz for a 20 MHz width channel with regulatory rule:
[ 25.830454] cfg80211: 2457000 KHz - 2482000 KHz @ KHz), (N/A mBi, 2000 mBm)
[ 25.830457] cfg80211: Disabling freq 2484 MHz as custom regd has no rule that fits a 20 MHz wide channel
[ 25.842012] cfg80211: Ignoring regulatory request Set by core since the driver uses its own custom regulatory domain
[ 26.048048] ieee80211 phy0: Selected rate control algorithm 'minstrel_ht'
[ 26.051154] ath5k phy0: Atheros AR2413 chip found (MAC: 0x78, PHY: 0x45)
[ 133.492866] ADDRCONF(NETDEV_UP): wlan0: link is not ready
[ 278.050097] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 1/3)
[ 278.248082] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 2/3)
[ 278.448111] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 3/3)
[ 278.648084] wlan0: direct probe to c0:3f:0e:b9:f3:b2 timed out
[ 283.920272] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 284.120232] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 284.320074] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 284.520081] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 295.029501] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 295.228085] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 295.428086] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 295.628099] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 306.137353] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 306.336092] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 306.536097] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 306.736099] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 317.245279] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 317.444097] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 317.644111] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 317.844091] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 328.348112] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 328.548171] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 328.748109] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 328.948095] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 339.453369] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 339.652085] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 339.852082] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 340.052092] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 344.316878] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 1/3)
[ 344.516064] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 2/3)
[ 344.716125] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 3/3)
[ 344.916101] wlan0: direct probe to c0:3f:0e:b9:f3:b2 timed out
[ 355.421554] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 1/3)
[ 355.620092] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 2/3)
[ 355.820642] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 3/3)
[ 356.020054] wlan0: direct probe to c0:3f:0e:b9:f3:b2 timed out
[ 366.521629] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 1/3)
[ 366.720085] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 2/3)
[ 366.920080] wlan0: direct probe to c0:3f:0e:b9:f3:b2 (try 3/3)
[ 367.120367] wlan0: direct probe to c0:3f:0e:b9:f3:b2 timed out
[ 433.347401] ADDRCONF(NETDEV_UP): wlan0: link is not ready
[ 468.143619] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 1/3)
[ 468.340073] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 2/3)
[ 468.540069] wlan0: direct probe to c2:3f:0e:b9:f3:b3 (try 3/3)
[ 468.740069] wlan0: direct probe to c2:3f:0e:b9:f3:b3 timed out
[ 482.093842] wlan0: authenticate with c2:3f:0e:b9:f3:b3 (try 1)
[ 482.095325] wlan0: authenticated
[ 482.095384] wlan0: associate with c2:3f:0e:b9:f3:b3 (try 1)
[ 482.097657] wlan0: RX AssocResp from c2:3f:0e:b9:f3:b3 (capab=0x401 status=0 aid=1)
[ 482.097666] wlan0: associated
[ 482.099748] ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
[ 492.416050] wlan0: no IPv6 routers present
[ 547.281613] wlan0: deauthenticating from c2:3f:0e:b9:f3:b3 by local choice (reason=3)
[ 547.281973] cfg80211: All devices are disconnected, going to restore regulatory settings
[ 547.281984] cfg80211: Restoring regulatory settings
[ 547.283060] cfg80211: Calling CRDA to update world regulatory domain
[ 547.311427] cfg80211: Ignoring regulatory request Set by core since the driver uses its own custom regulatory domain
[ 547.311446] cfg80211: World regulatory domain updated:
[ 547.311451] cfg80211: (start_freq - end_freq @ bandwidth), (max_antenna_gain, max_eirp)
[ 547.311461] cfg80211: (2402000 KHz - 2472000 KHz @ 40000 KHz), (300 mBi, 2000 mBm)
[ 547.311469] cfg80211: (2457000 KHz - 2482000 KHz @ 20000 KHz), (300 mBi, 2000 mBm)
[ 547.311477] cfg80211: (2474000 KHz - 2494000 KHz @ 20000 KHz), (300 mBi, 2000 mBm)
[ 547.311484] cfg80211: (5170000 KHz - 5250000 KHz @ 40000 KHz), (300 mBi, 2000 mBm)
[ 547.311492] cfg80211: (5735000 KHz - 5835000 KHz @ 40000 KHz), (300 mBi, 2000 mBm)
^ permalink raw reply
* Re: [PATCH] veth: use batched device unregister
From: Michał Mirosław @ 2011-05-09 6:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Alex Bligh, netdev, Jesse Gross, Paul E. McKenney,
Ben Greear
In-Reply-To: <1304916275.3207.79.camel@edumazet-laptop>
2011/5/9 Eric Dumazet <eric.dumazet@gmail.com>:
> veth devices dont use the batched device unregisters yet.
>
> Since veth are a pair of devices, it makes sense to use a batch of two
> unregisters, this roughly divide dismantle time by two.
[...]
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -451,8 +451,16 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
> priv = netdev_priv(dev);
> peer = priv->peer;
>
> - unregister_netdevice_queue(dev, head);
> - unregister_netdevice_queue(peer, head);
> + if (head == NULL) {
> + LIST_HEAD(list);
> + /* make a batch of two devices to speedup unregister */
> + unregister_netdevice_queue(dev, &list);
> + unregister_netdevice_queue(peer, &list);
> + unregister_netdevice_many(&list);
> + } else {
> + unregister_netdevice_queue(dev, head);
> + unregister_netdevice_queue(peer, head);
> + }
You could change dellink callers to always pass head != NULL. As a
side effect, unregister_netdevice_queue() would do just what its name
suggests.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Scalability of interface creation and deletion
From: Eric Dumazet @ 2011-05-09 6:37 UTC (permalink / raw)
To: Alex Bligh; +Cc: paulmck, netdev, Jesse Gross, Ben Greear
In-Reply-To: <BA03B09F0474B30660A091FD@nimrod.local>
Le lundi 09 mai 2011 à 06:37 +0100, Alex Bligh a écrit :
>
> --On 8 May 2011 23:00:47 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > We can probably speed things, but we should consider the following user
> > actions :
>
> How about
>
> > ip link add link eth0 vlan103 type vlan id 103
> > ip link del vlan103
>
> Removes and unlinks structures, including making name available, sending
> out netlink messages, but doesn't free things
Most of the cleanup work has to be done with RTNL being held, and this
might because of transaction atomicity requirement.
In your test you dismantle idle devices. Now think a bit when you have
both trafic in and out, sockets with destinations still pointing to the
device, in flight arp requests, all this using RCU of course.
When you dismantle one device (or several in case of a module unload),
this can have implications on other devices (see veth cas for an obvious
example : this automatically removes the peer device), but also on
routes, neighbours, cached routes, various protocol cleanups, ... and so
on. Few people even on netdev understand the whole picture.
Given that 99.99% machines setup netdevice at boot time only, and hardly
consider dismantles, we netdev guys were pragmatic and safe. Two or
three synchronize_rcu() were considered as a non issue.
It seems there is interest to improve things now.
One way is to allow more batching and delegation, and I am working on
that right now, using a kthread, so that we dont block the requester for
the whole device dismantle.
This kthread might use call_rcu() driven state machine, but that is a
detail of implementation, since only kthread would be impacted.
I am pretty busy at work these days, so dont expect patches before some
time :)
^ permalink raw reply
* Re: [PATCH 3/3] virtio_ring: need_event api comment fix
From: Rusty Russell @ 2011-05-09 5:59 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Krishna Kumar, Carsten Otte, lguest-uLR06cmDAlY/bJ5BZ2RsiQ,
Shirley Ma, kvm-u79uwXL29TY76Z2rM5mHXA,
linux-s390-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
steved-r/Jw6+rmf7HQT0dZR+AlfA, Christian Borntraeger,
Tom Lendacky, Martin Schwidefsky, linux390-tA70FqPdS9bQT0dZR+AlfA
In-Reply-To: <c8b6d8a8df4927826d0024fceeb68726c50d0b1a.1304605817.git.mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, 5 May 2011 18:08:17 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> fix typo in a comment: size -> side
>
> Reported-by: Stefan Hajnoczi <stefanha-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
I could smerge these together for you, but I *really* want benchmarks in
these commit messages.
Thanks,
Rusty.
PS. Was away last week, hence the delay on this...
^ permalink raw reply
* Re: [PATCH 14/18] virtio: add api for delayed callbacks
From: Rusty Russell @ 2011-05-09 5:57 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Krishna Kumar, Carsten Otte, lguest-uLR06cmDAlY/bJ5BZ2RsiQ,
Shirley Ma, kvm-u79uwXL29TY76Z2rM5mHXA,
linux-s390-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
steved-r/Jw6+rmf7HQT0dZR+AlfA, Christian Borntraeger,
Tom Lendacky, Martin Schwidefsky, linux390-tA70FqPdS9bQT0dZR+AlfA
In-Reply-To: <64d47c628b3fdc0ac156aed4be182933d8bcc0db.1304541919.git.mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, 4 May 2011 23:52:33 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Add an API that tells the other side that callbacks
> should be delayed until a lot of work has been done.
> Implement using the new used_event feature.
Since you're going to add a capacity query anyway, why not add the
threshold argument here?
Then the caller can choose how much space it needs. Maybe net and block
will want different things...
Cheers,
Rusty.
^ permalink raw reply
* [PATCH] rtlwifi: rtl8192cu: Fix memset using sizeof(ptr) not sizeof(*ptr)
From: Joe Perches @ 2011-05-09 5:43 UTC (permalink / raw)
To: Larry Finger, Chaoming Li; +Cc: John W. Linville, linux-wireless, netdev, LKML
Found via coccinelle script
@@
type T;
T* ptr;
expression E1;
@@
* memset(E1, 0, sizeof(ptr));
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/wireless/rtlwifi/rtl8192cu/trx.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/trx.c b/drivers/net/wireless/rtlwifi/rtl8192cu/trx.c
index 79c98f6..c16fc1c 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192cu/trx.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192cu/trx.c
@@ -372,7 +372,7 @@ static void _rtl_rx_process(struct ieee80211_hw *hw, struct sk_buff *skb)
__le16 fc;
struct ieee80211_hdr *hdr;
- memset(rx_status, 0, sizeof(rx_status));
+ memset(rx_status, 0, sizeof(*rx_status));
rxdesc = skb->data;
skb_len = skb->len;
drvinfo_len = (GET_RX_DESC_DRVINFO_SIZE(rxdesc) * RTL_RX_DRV_INFO_UNIT);
^ permalink raw reply related
* Re: Scalability of interface creation and deletion
From: Alex Bligh @ 2011-05-09 5:37 UTC (permalink / raw)
To: Eric Dumazet, paulmck; +Cc: netdev, Jesse Gross, Alex Bligh
In-Reply-To: <1304888447.3207.66.camel@edumazet-laptop>
--On 8 May 2011 23:00:47 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote:
> We can probably speed things, but we should consider the following user
> actions :
How about
> ip link add link eth0 vlan103 type vlan id 103
> ip link del vlan103
Removes and unlinks structures, including making name available, sending
out netlink messages, but doesn't free things
> ip link add link eth1 vlan103 type vlan id 103
creates new interface
[some time later] original zombie i/f freed
> The "link del" command should return to user only if the minimum things
> had been done, to make sure the following "link add" wont fail
> mysteriously.
Are you worried about failure through name collision (already
dealt with), vlan tag collision (ditto) or what?
--
Alex Bligh
^ permalink raw reply
* [PATCH] veth: use batched device unregister
From: Eric Dumazet @ 2011-05-09 4:44 UTC (permalink / raw)
To: David Miller
Cc: Alex Bligh, netdev, Jesse Gross, Paul E. McKenney, Ben Greear
In-Reply-To: <1304888447.3207.66.camel@edumazet-laptop>
veth devices dont use the batched device unregisters yet.
Since veth are a pair of devices, it makes sense to use a batch of two
unregisters, this roughly divide dismantle time by two.
Reported-by: Alex Bligh <alex@alex.org.uk>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jesse Gross <jesse@nicira.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ben Greear <greearb@candelatech.com>
---
drivers/net/veth.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 3b99f64..77c4679 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -451,8 +451,16 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
priv = netdev_priv(dev);
peer = priv->peer;
- unregister_netdevice_queue(dev, head);
- unregister_netdevice_queue(peer, head);
+ if (head == NULL) {
+ LIST_HEAD(list);
+ /* make a batch of two devices to speedup unregister */
+ unregister_netdevice_queue(dev, &list);
+ unregister_netdevice_queue(peer, &list);
+ unregister_netdevice_many(&list);
+ } else {
+ unregister_netdevice_queue(dev, head);
+ unregister_netdevice_queue(peer, head);
+ }
}
static const struct nla_policy veth_policy[VETH_INFO_MAX + 1];
^ permalink raw reply related
* Re: [PATCH 09/18] virtio: use avail_event index
From: Rusty Russell @ 2011-05-09 4:33 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Krishna Kumar, Carsten Otte, lguest-uLR06cmDAlY/bJ5BZ2RsiQ,
Shirley Ma, kvm-u79uwXL29TY76Z2rM5mHXA,
linux-s390-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
steved-r/Jw6+rmf7HQT0dZR+AlfA, Christian Borntraeger,
Tom Lendacky, Martin Schwidefsky, linux390-tA70FqPdS9bQT0dZR+AlfA
In-Reply-To: <8bba6a0a8eee17e741c5155b04ff1b1c9f34bf94.1304541919.git.mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Use the new avail_event feature to reduce the number
> of exits from the guest.
Figures here would be nice :)
> @@ -228,6 +237,12 @@ add_head:
> * new available array entries. */
> virtio_wmb();
> vq->vring.avail->idx++;
> + /* If the driver never bothers to kick in a very long while,
> + * avail index might wrap around. If that happens, invalidate
> + * kicked_avail index we stored. TODO: make sure all drivers
> + * kick at least once in 2^16 and remove this. */
> + if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
> + vq->kicked_avail_valid = true;
If they don't, they're already buggy. Simply do:
WARN_ON(vq->vring.avail->idx == vq->kicked_avail);
> +static bool vring_notify(struct vring_virtqueue *vq)
> +{
> + u16 old, new;
> + bool v;
> + if (!vq->event)
> + return !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +
> + v = vq->kicked_avail_valid;
> + old = vq->kicked_avail;
> + new = vq->kicked_avail = vq->vring.avail->idx;
> + vq->kicked_avail_valid = true;
> + if (unlikely(!v))
> + return true;
This is the only place you actually used kicked_avail_valid. Is it
possible to initialize it in such a way that you can remove this?
> @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device *vdev)
> break;
> case VIRTIO_RING_F_USED_EVENT_IDX:
> break;
> + case VIRTIO_RING_F_AVAIL_EVENT_IDX:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
Does this belong in a prior patch?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 08/18] virtio_ring: support for used_event idx feature
From: Rusty Russell @ 2011-05-09 4:17 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Krishna Kumar, Carsten Otte, lguest-uLR06cmDAlY/bJ5BZ2RsiQ,
Shirley Ma, kvm-u79uwXL29TY76Z2rM5mHXA,
linux-s390-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
steved-r/Jw6+rmf7HQT0dZR+AlfA, Christian Borntraeger,
Tom Lendacky, Martin Schwidefsky, linux390-tA70FqPdS9bQT0dZR+AlfA
In-Reply-To: <dd16b9f3bad847b0eadc7f94368e27982c1a7560.1304541919.git.mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, 4 May 2011 23:51:38 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Add support for the used_event idx feature: when enabling
> interrupts, publish the current avail index value to
> the host so that we get interrupts on the next update.
>
> Signed-off-by: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/virtio/virtio_ring.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 507d6eb..3a3ed75 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> ret = vq->data[i];
> detach_buf(vq, i);
> vq->last_used_idx++;
> + /* If we expect an interrupt for the next entry, tell host
> + * by writing event index and flush out the write before
> + * the read in the next get_buf call. */
> + if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
> + vring_used_event(&vq->vring) = vq->last_used_idx;
> + virtio_mb();
> + }
> +
Hmm, so you're still using the avail->flags; it's just if thresholding
is enabled the host will ignore it?
It's a little subtle, but it keeps this patch small. Perhaps we'll want
to make it more explicit later.
Thanks,
Rusty.
^ permalink raw reply
* Re: linux-next: build warning after merge of the net tree
From: David Miller @ 2011-05-09 4:13 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel
In-Reply-To: <20110509134613.bcf0b017.sfr@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Mon, 9 May 2011 13:46:13 +1000
> Hi all,
>
> After merging the net tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
>
> net/sctp/protocol.c: In function 'sctp_v4_xmit':
> net/sctp/protocol.c:842: warning: format '%p' expects type 'void *', but argument 5 has type '__be32'
> net/sctp/protocol.c:842: warning: format '%p' expects type 'void *', but argument 6 has type '__be32'
>
> Caused by commit f1c0a276ea17 ("sctp: Don't use rt->rt_{src,dst} in
> sctp_v4_xmit()").
I'll fix this, thanks for the report Stephen.
^ permalink raw reply
* Re: [PATCH 06/18] virtio_ring: avail event index interface
From: Rusty Russell @ 2011-05-09 4:13 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Carsten Otte, Christian Borntraeger, linux390, Martin Schwidefsky,
Heiko Carstens, Shirley Ma, lguest, linux-kernel, virtualization,
netdev, linux-s390, kvm, Krishna Kumar, Tom Lendacky, steved,
habanero
In-Reply-To: <b00ef5d572ec9bc3b884c496025512abc1f14349.1304541919.git.mst@redhat.com>
On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Define a new feature bit for the host to
> declare that it uses an avail_event index
> (like Xen) instead of a feature bit
> to enable/disable interrupts.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/virtio_ring.h | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index f5c1b75..f791772 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -32,6 +32,9 @@
> /* The Guest publishes the used index for which it expects an interrupt
> * at the end of the avail ring. Host should ignore the avail->flags field. */
> #define VIRTIO_RING_F_USED_EVENT_IDX 29
> +/* The Host publishes the avail index for which it expects a kick
> + * at the end of the used ring. Guest should ignore the used->flags field. */
> +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32
Are you really sure we want to separate the two? Seems a little simpler
to have one bit to mean "we're publishing our threshold". For someone
implementing this from scratch, it's a little simpler.
Or are there cases where the old style makes more sense?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/2] ipv4: Pass flow keys down into datagram packet building engine.
From: David Miller @ 2011-05-09 4:07 UTC (permalink / raw)
To: joe; +Cc: netdev
In-Reply-To: <Pine.LNX.4.30.1105081811570.26257-100000@mail.perches.com>
From: Joe Perches <joe@perches.com>
Date: Sun, 8 May 2011 18:14:46 -0700 (PDT)
> On Sun, 8 May 2011, David Miller wrote:
>> +static struct rtable *icmp_route_lookup(struct net *net,
>> + struct flowi4 *fl4,
>> + struct sk_buff *skb_in,
>> const struct iphdr *iph,
>> __be32 saddr, u8 tos,
>> int type, int code,
>> struct icmp_bxm *param)
>> {
> []
>> + memset(&fl4, 0, sizeof(fl4));
>
> memset(fl4, 0, sizeof(*fl4));
>
>> + fl4->daddr = (param->replyopts.opt.opt.srr ?
>> + param->replyopts.opt.opt.faddr : iph->saddr);
>
Thanks Joe :-/ I'll fix that.
^ permalink raw reply
* linux-next: build warning after merge of the net tree
From: Stephen Rothwell @ 2011-05-09 3:46 UTC (permalink / raw)
To: David Miller, netdev; +Cc: linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 568 bytes --]
Hi all,
After merging the net tree, today's linux-next build (x86_64 allmodconfig)
produced this warning:
net/sctp/protocol.c: In function 'sctp_v4_xmit':
net/sctp/protocol.c:842: warning: format '%p' expects type 'void *', but argument 5 has type '__be32'
net/sctp/protocol.c:842: warning: format '%p' expects type 'void *', but argument 6 has type '__be32'
Caused by commit f1c0a276ea17 ("sctp: Don't use rt->rt_{src,dst} in
sctp_v4_xmit()").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: future developments of usbnet
From: Ming Lei @ 2011-05-09 3:26 UTC (permalink / raw)
To: Oliver Neukum
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <201105062045.37336.oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Hi,
2011/5/7 Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>:
> Hi,
>
> I'd like to get a feeling what people are working out there regarding usbnet.
> So please, if you do something, or think something ought to be done, please
> speak up now.
>
> IMHO usbnet needs better support for
>
> - batching protocols
> - double buffering on the rx path
>
> with the latter having higher priority.
>
> Coments?
Maybe another thing about rx should be considered too: we should introduce
one flow control mechanism into rx path so that too much coming packets can
not consume many of memory and cause out of memory for atomic allocation.
Current implementation has such kind of risk certainly, as i explained in the
patch with title below:
usbnet: runtime pm: fix out of memory
thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v2 000/115] faster tree-based sysctl implementation
From: Eric W. Biederman @ 2011-05-09 3:11 UTC (permalink / raw)
To: Lucian Adrian Grijincu
Cc: linux-kernel, netdev, Alexey Dobriyan, Octavian Purdila,
David S . Miller
In-Reply-To: <1304894407-32201-1-git-send-email-lucian.grijincu@gmail.com>
Lucian Adrian Grijincu <lucian.grijincu@gmail.com> writes:
> Eric also asked for:
> - replacing the per-header list of subdirs with a rbtree.
> -- Again, lack of time, and this can always be added at a later time
> to optimize lookup and duplicate checks. At the moment this patch
> series does not add a complexity regression over the previous
> implementation, au contraire.
Instead you add a dubious mechanisms, to avoid duplicates checks
because the duplicate checks takes time.
A good internal data structure should have logN duplicate checks
and that should not be cost prohibitive. For maintainability of
the users a good internal data structure removes the need for
special cases, and it actually makes use the sysctl fast.
Your implementation continues to have a O(N^2) cost to use the
sysctl files.
Getting a good internal data structure is the most important part
of a making sysctls fast, and keeping them fast. Your patchset
fails in that.
So at that level, a great big NAK on the patchset as it currently
exists.
You have made a key observation that we only need better data
structures for the directories and can leave off that work
for the callers.
Thank you for that.
Additionally thank you for splitting up your changes to the
core of sysctl so there is a chance of reviewing those.
With respect to review and merging. The priorities should be:
1) Figure out what the users really need to do, before we change
everything so we only need one sweeping pass through the users
that hopefully simplifies them.
2) Ensure you have a version of the new interfaces ready at the
start of the patchset, so the sweeping changes can be made
incrementally.
3) Clean up the users.
4) Make simplifications because you don't have any old users left.
I very much agree that the data structures that sysctl uses today
are far from ideal, and that we can make a lot of headway.
That said I think there is some good work in your patchset and I may
try and cherry pick out the good bits that we can merge now.
Eric
^ permalink raw reply
* Re: [PATCH] net/bonding: adjust codingstyle for bond_3ad files
From: Håkon Løvdal @ 2011-05-09 1:30 UTC (permalink / raw)
To: David Miller
Cc: mfmooney, joe, aquini, kernel-janitors, fubar, andy, shemminger,
netdev, nikai
In-Reply-To: <20110508.171204.27789908.davem@davemloft.net>
On 9 May 2011 02:12, David Miller <davem@davemloft.net> wrote:
> From: Håkon Løvdal <hlovdal@gmail.com>
> Date: Mon, 9 May 2011 02:08:41 +0200
>
>> void bond_3ad_state_machine_handler(struct work_struct *work)
>> {
>> struct bonding *bond = container_of(work, struct bonding,
>> ad_work.work);
>> struct port *port;
>> struct aggregator *aggregator;
>>
>> read_lock(&bond->lock);
>>
>> if (! bond->kill_timers) {
>>
>> //check if there are any slaves
>> if (bond->slave_cnt != 0) {
>> ...
>> }
>> queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
>> }
>> read_unlock(&bond->lock);
>> }
>>
>>
>> And this was what I trying to reccommend against (and which the
>> stackoverflow question is about).
>
> I really don't see anything wrong with either approach.
The example above looks innocent since it is so small. But written
this way, the all of core of the function ("...") will have two extra,
unnecessary indentation levels. My rule of thumb is to always test for
exceptions, not the normal case.
Expanding the example to cover the full function, it could be like the
following:
void bond_3ad_state_machine_handler(struct work_struct *work)
{
struct bonding *bond = container_of(work, struct bonding,
ad_work.work);
struct port *port;
struct aggregator *aggregator;
read_lock(&bond->lock);
if (! bond->kill_timers)
{
//check if there are any slaves
if (bond->slave_cnt != 0)
{
int uninitialized = 0;
// check if agg_select_timer timer after
initialize is timed out
if (BOND_AD_INFO(bond).agg_select_timer &&
!(--BOND_AD_INFO(bond).agg_select_timer)) {
// select the active aggregator for the bond
if ((port = __get_first_port(bond))) {
if (port->slave) {
aggregator =
__get_first_agg(port);
ad_agg_selection_logic(aggregator);
} else {
pr_warning("%s:
Warning: bond's first port is uninitialized\n",
bond->dev->name);
uninitialized = 1;
}
}
if (! uninitialized)|
bond_3ad_set_carrier(bond);
}
if (! uninitialized) {
// for each port run the state machines
for (port = __get_first_port(bond);
port; port = __get_next_port(port)) {
if (port->slave) {
/* Lock around state
machines to protect data accessed
* by all (e.g.,
port->sm_vars). ad_rx_machine may run
* concurrently due to
incoming LACPDU.
*/
__get_state_machine_lock(port);
ad_rx_machine(NULL, port);
ad_periodic_machine(port);
ad_port_selection_logic(port);
ad_mux_machine(port);
ad_tx_machine(port);
// turn off the BEGIN
bit, since we already handled it
if (port->sm_vars &
AD_PORT_BEGIN)
port->sm_vars
&= ~AD_PORT_BEGIN;
__release_state_machine_lock(port);
} else {
pr_warning("%s:
Warning: Found an uninitialized port\n",
bond->dev->name);
}
}
}
}
queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
}
read_unlock(&bond->lock);
}
Notice for instance "aggregator = __get_first_agg(port);" now have 5
levels of indentation in addition to the function body level, compared
to just 2 in the original.
That extra indentation is one thing, but the most important in my opinion
is that layout of the code becomes harder to read. This is because the
error handling code will typically be much more intrusive in the "layout".
Good code written like
if (error_check) {
error
handling
code
}
normal
code
here
is possible to quickly scan/read, but if code is written like
if (some_check) {
error
handling
code
} else {
normal
code
here
}
or
if (some_check) {
normal
code
here
} else {
error
handling
code
}
you have to go into slow gear and read and mentally process (relatively)
much more of the code to get the same picture. Combine a few such if/else,
nested - now everything is tightly intervened and impossible to quickly
grasp and/or hold in your head.
And the "uninitialized" variable is a crutch.
BR Håkon Løvdal
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox