| Message ID | 1385843940.2664.4.camel@joe-AO722 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 12/01/2013 04:39 AM, Joe Perches wrote: > On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote: >> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote: >>> - fall-through is obvious (although I did not notice it, originally). >>> >>> - Check 'A' again just near by "case A" seems a little strange. >>> >>> - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK. >> >> I know. If you have any good ideas of how to make it more obvious to the >> compiler, I'm all ears, I just don't like any of the solutions offered >> so far (and you aren't the first to do so either) :-) >> OK, thanks. >> FWIW, I find the label to be odd because if you're familiar with the >> code then AP/AP_VLAN *should* be identical except for two special things >> that are now linearly & neatly handled in the code (the first being the >> 4-addr station, the second the chanctx assignment which always has to be >> done regardless of 4-addr). IMHO the == check after case should be >> enough to make a human reader take a closer look. I understand that you >> didn't and that's OK since you were just trying to squelch compile >> warnings, but I don't see that this one warrants much attention. > > The label/test could be moved to save a couple of lines > of duplicated code. > Hmm... for me, it must be an implementation which can satisfy all of us. If ieee80211_subif_start_xmit() is not performance sensitive (I guess so), we can use some short static functions instead of some code blocks within ieee80211_subif_start_xmit(). - ieee80211_subif_start_xmit() is a long function (600+ lines). - use short static function can share some code. - if code can be shared, the work flow can be more clearer too (don't need fall-through or goto). And I guess, the implementation below can cause panic when "sdata->vif.type == NL80211_IFTYPE_AP_VLAN". :-( > Maybe: > > net/mac80211/tx.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index 70b5a05..b2160f4 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, > } > ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, > u.ap); > - chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); > - if (!chanctx_conf) > - goto fail_rcu; > - band = chanctx_conf->def.chan->band; > - if (sta) > - break; > /* fall through */ > case NL80211_IFTYPE_AP: > - if (sdata->vif.type == NL80211_IFTYPE_AP) > - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); > + chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); > if (!chanctx_conf) > goto fail_rcu; > + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { > + band = chanctx_conf->def.chan->band; > + if (sta) > + break; > + } > fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS); > /* DA BSSID SA */ > memcpy(hdr.addr1, skb->data, ETH_ALEN); > >
On 12/01/2013 07:48 AM, Chen Gang wrote: > On 12/01/2013 04:39 AM, Joe Perches wrote: >> On Sat, 2013-11-30 at 21:08 +0100, Johannes Berg wrote: >>> On Sat, 2013-11-30 at 22:02 +0800, Chen Gang wrote: >>>> - fall-through is obvious (although I did not notice it, originally). >>>> >>>> - Check 'A' again just near by "case A" seems a little strange. >>>> >>>> - Some compilers aren't quit smart enough to know 'chanctx_conf' is OK. >>> >>> I know. If you have any good ideas of how to make it more obvious to the >>> compiler, I'm all ears, I just don't like any of the solutions offered >>> so far (and you aren't the first to do so either) :-) >>> > > OK, thanks. > >>> FWIW, I find the label to be odd because if you're familiar with the >>> code then AP/AP_VLAN *should* be identical except for two special things >>> that are now linearly & neatly handled in the code (the first being the >>> 4-addr station, the second the chanctx assignment which always has to be >>> done regardless of 4-addr). IMHO the == check after case should be >>> enough to make a human reader take a closer look. I understand that you >>> didn't and that's OK since you were just trying to squelch compile >>> warnings, but I don't see that this one warrants much attention. >> >> The label/test could be moved to save a couple of lines >> of duplicated code. >> > > Hmm... for me, it must be an implementation which can satisfy all of us. > > If ieee80211_subif_start_xmit() is not performance sensitive (I guess > so), we can use some short static functions instead of some code blocks > within ieee80211_subif_start_xmit(). > > - ieee80211_subif_start_xmit() is a long function (600+ lines). > Oh, one typo issue (need use 400+ instead of 600+), it is a long function (although not quite long). > - use short static function can share some code. > > - if code can be shared, the work flow can be more clearer too (don't > need fall-through or goto). > > > And I guess, the implementation below can cause panic when > "sdata->vif.type == NL80211_IFTYPE_AP_VLAN". :-( > >> Maybe: >> >> net/mac80211/tx.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c >> index 70b5a05..b2160f4 100644 >> --- a/net/mac80211/tx.c >> +++ b/net/mac80211/tx.c >> @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, >> } >> ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, >> u.ap); >> - chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); >> - if (!chanctx_conf) >> - goto fail_rcu; >> - band = chanctx_conf->def.chan->band; >> - if (sta) >> - break; >> /* fall through */ >> case NL80211_IFTYPE_AP: >> - if (sdata->vif.type == NL80211_IFTYPE_AP) >> - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); >> + chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); >> if (!chanctx_conf) >> goto fail_rcu; >> + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { >> + band = chanctx_conf->def.chan->band; >> + if (sta) >> + break; >> + } >> fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS); >> /* DA BSSID SA */ >> memcpy(hdr.addr1, skb->data, ETH_ALEN); >> >> > >
On Sat, 2013-11-30 at 12:39 -0800, Joe Perches wrote: > +++ b/net/mac80211/tx.c > @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, > } > ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, > u.ap); > - chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); > - if (!chanctx_conf) > - goto fail_rcu; > - band = chanctx_conf->def.chan->band; > - if (sta) > - break; > /* fall through */ > case NL80211_IFTYPE_AP: > - if (sdata->vif.type == NL80211_IFTYPE_AP) > - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); > + chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); Good try, but no, now ap_sdata isn't even assigned. :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote: > If ieee80211_subif_start_xmit() is not performance sensitive (I guess > so), we can use some short static functions instead of some code blocks > within ieee80211_subif_start_xmit(). > > - ieee80211_subif_start_xmit() is a long function (600+ lines). > > - use short static function can share some code. > > - if code can be shared, the work flow can be more clearer too (don't > need fall-through or goto). Frankly, I'm getting tired of discussing this. Please don't try to rewrite this code until you've understood it. You suggesting that "start_xmit()" isn't a performance sensitive function makes me realize you haven't even tried. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/01/2013 05:37 PM, Johannes Berg wrote: > On Sun, 2013-12-01 at 07:48 +0800, Chen Gang wrote: > >> If ieee80211_subif_start_xmit() is not performance sensitive (I guess >> so), we can use some short static functions instead of some code blocks >> within ieee80211_subif_start_xmit(). >> >> - ieee80211_subif_start_xmit() is a long function (600+ lines). >> >> - use short static function can share some code. >> >> - if code can be shared, the work flow can be more clearer too (don't >> need fall-through or goto). > > Frankly, I'm getting tired of discussing this. Please don't try to > rewrite this code until you've understood it. You suggesting that > "start_xmit()" isn't a performance sensitive function makes me realize > you haven't even tried. > OK, thank you for "you are getting tired ...". Please help try when you have time. :-) No I didn't try -- so use 'if' and 'guess' for discussing. Hmm... if it is performance sensitive: - use static function instead of some code block and try to share them. it adds additional instructions, but can shrink binary code size. if the performance is acceptable, it is the best way to me. - else (1st way not acceptable), use macro instead of static function. it expends binary code size, but can save some instructions. if the performance is acceptable, it is the acceptable way to me. - If neither of static function nor macro are acceptable, I still prefer to use 'goto' instead of 'fall-through'. that can let all compilers feel well, and can not feel any strange. normally, when prev case uses fall-through, it need be sure of next case need not notice about it (prev case). or it will make a little strange for readers when read next case. Welcome any suggestions or completions. Thanks.
On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote:
> Good try, but no, now ap_sdata isn't even assigned. :)
Right. Oh well. There's no improving this without
significant rewrite. Even then, there may not be much
overall improvement.
btw: Chen, I think fall-throughs are fine as long as
they are commented appropriately.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2013 06:38 AM, Joe Perches wrote: > On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote: >> Good try, but no, now ap_sdata isn't even assigned. :) > > Right. Oh well. There's no improving this without > significant rewrite. Even then, there may not be much > overall improvement. > > btw: Chen, I think fall-throughs are fine as long as > they are commented appropriately. > Hmm... for me, when use fall-through, need 2 things: - need a comment. - next case need not notice about prev case which fall-through. If we can not fit the 2 things above, either can not use sub functions or macros for it because of performance reason, 'goto' is better than 'fall-through'. For me, in most cases, when a function becomes a long function, it is always better to use sub-fuctions or macros instead of some blocks (but it is really not urgent :-) ). Thanks.
On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote: > On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote: > > Good try, but no, now ap_sdata isn't even assigned. :) > > Right. Oh well. There's no improving this without > significant rewrite. Even then, there may not be much > overall improvement. I could see an improvement if we were to actually make changes to assign the pointer based on the iftype, when that changes, so that each function only has to handle the right type and we don't need the switch statement at all... But that's tricky to get right and I'm between vacations and holidays and all that ... johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2013 10:48 PM, Johannes Berg wrote: > On Sun, 2013-12-01 at 14:38 -0800, Joe Perches wrote: >> On Sun, 2013-12-01 at 10:35 +0100, Johannes Berg wrote: >>> Good try, but no, now ap_sdata isn't even assigned. :) >> >> Right. Oh well. There's no improving this without >> significant rewrite. Even then, there may not be much >> overall improvement. > > I could see an improvement if we were to actually make changes to assign > the pointer based on the iftype, when that changes, so that each > function only has to handle the right type and we don't need the switch > statement at all... > OK, thanks, it is one of good ideas to me (which I can not find). :-) > But that's tricky to get right and I'm between vacations and holidays > and all that ... > It is really not urgent, and for keeping quality, it is necessary to spend suitable time resource (e.g 1 hour or more) to make, review and test this kind of patch carefully by oneself. So could you please help improve it when you have suitable related time resources? Thanks.
On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote: > It is really not urgent, and for keeping quality, it is necessary to > spend suitable time resource (e.g 1 hour or more) to make, review and > test this kind of patch carefully by oneself. > > So could you please help improve it when you have suitable related time > resources? No. Who are you to suggest what I need to work on? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2013 04:04 PM, Johannes Berg wrote: > On Wed, 2013-12-04 at 10:12 +0800, Chen Gang wrote: > >> It is really not urgent, and for keeping quality, it is necessary to >> spend suitable time resource (e.g 1 hour or more) to make, review and >> test this kind of patch carefully by oneself. >> >> So could you please help improve it when you have suitable related time >> resources? > > No. Who are you to suggest what I need to work on? > > johannes > I am one of volunteers for Open Source, and I don't care about who I am. Since this thread is started by me, I have duty to try my best to let it finish (but may fail). According to our original discussion, it seems we agree that I am not the suitable member to finish it, so I suggest you or another members to try. Thanks.
On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote: > According to our original discussion, it seems we agree that I am not > the suitable member to finish it, so I suggest you or another members to > try. There's nothing to finish here. The code is fine. The compiler is wrong, but we haven't found a way to shut up the compiler without breaking the code. Please let's just let it rest. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2013 04:49 PM, Johannes Berg wrote: > On Wed, 2013-12-04 at 16:41 +0800, Chen Gang wrote: > >> According to our original discussion, it seems we agree that I am not >> the suitable member to finish it, so I suggest you or another members to >> try. > > There's nothing to finish here. The code is fine. The compiler is wrong, > but we haven't found a way to shut up the compiler without breaking the > code. Please let's just let it rest. > For me, I still stick to the code can be improved, although it is not urgent. But just like you said, we can just stop discussing -- every members can save their own opinions. And I am not the related maintainer, so if no additional suggestions, discussions or completions, I will/should stop here. Thanks.
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 70b5a05..b2160f4 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1777,18 +1777,16 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, } ap_sdata = container_of(sdata->bss, struct ieee80211_sub_if_data, u.ap); - chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); - if (!chanctx_conf) - goto fail_rcu; - band = chanctx_conf->def.chan->band; - if (sta) - break; /* fall through */ case NL80211_IFTYPE_AP: - if (sdata->vif.type == NL80211_IFTYPE_AP) - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf); + chanctx_conf = rcu_dereference(ap_sdata->vif.chanctx_conf); if (!chanctx_conf) goto fail_rcu; + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { + band = chanctx_conf->def.chan->band; + if (sta) + break; + } fc |= cpu_to_le16(IEEE80211_FCTL_FROMDS); /* DA BSSID SA */ memcpy(hdr.addr1, skb->data, ETH_ALEN);