| Message ID | alpine.LNX.2.00.1111132247440.3368@swampdragon.chaosbits.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jesper Juhl <jj@chaosbits.net> Date: Sun, 13 Nov 2011 22:55:50 +0100 (CET) > We test for 'tx_ring' being != zero and BUG() if that's the case. So after > that check there is no way that 'tx_ring' could be anything _but_ zero, so > testing it again is just dead code. Once that dead code is removed, the > 'pkc' local variable becomes entirely redundant, so remove that as well. > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> Applied, thanks Jesper. -- 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 14/11/11 08:55, Jesper Juhl wrote: > We test for 'tx_ring' being != zero and BUG() if that's the case. So after > that check there is no way that 'tx_ring' could be anything _but_ zero, so > testing it again is just dead code. Once that dead code is removed, the > 'pkc' local variable becomes entirely redundant, so remove that as well. What if CONFIG_BUG=n? ~Ryan > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > net/packet/af_packet.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > only compile tested. > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 82a6f34..ab10e84 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -516,13 +516,11 @@ static void prb_init_blk_timer(struct packet_sock *po, > > static void prb_setup_retire_blk_timer(struct packet_sock *po, int tx_ring) > { > - struct tpacket_kbdq_core *pkc; > - > if (tx_ring) > BUG(); > > - pkc = tx_ring ? &po->tx_ring.prb_bdqc : &po->rx_ring.prb_bdqc; > - prb_init_blk_timer(po, pkc, prb_retire_rx_blk_timer_expired); > + prb_init_blk_timer(po, &po->rx_ring.prb_bdqc, > + prb_retire_rx_blk_timer_expired); > } > > static int prb_calc_retire_blk_tmo(struct packet_sock *po, -- 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 Sun, Nov 13, 2011 at 4:55 PM, Jesper Juhl <jj@chaosbits.net> wrote: > We test for 'tx_ring' being != zero and BUG() if that's the case. So after > that check there is no way that 'tx_ring' could be anything _but_ zero, so > testing it again is just dead code. Once that dead code is removed, the > 'pkc' local variable becomes entirely redundant, so remove that as well. It was there so that it would be a no-brainer thing for the next person who would want to enable tx_ring support. And given that this check was called just once during the initial setup, it didn't seem like a big deal to me. And the BUG() would let the next-person know what different paths need to be plugged. Chetan Loke -- 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 Mon, 14 Nov 2011, Ryan Mallon wrote: > On 14/11/11 08:55, Jesper Juhl wrote: > > > We test for 'tx_ring' being != zero and BUG() if that's the case. So after > > that check there is no way that 'tx_ring' could be anything _but_ zero, so > > testing it again is just dead code. Once that dead code is removed, the > > 'pkc' local variable becomes entirely redundant, so remove that as well. > > > What if CONFIG_BUG=n? > Arrgh, I didn't consider that (should have, but didn't).. In that case we'll have #define BUG() do {} while(0) which is not going to work all that peachy with my change... David: You may want to pass on this one. I obviously didn't think it through properly - sorry :-(
On 15/11/11 10:37, Jesper Juhl wrote: > On Mon, 14 Nov 2011, Ryan Mallon wrote: > >> On 14/11/11 08:55, Jesper Juhl wrote: >> >>> We test for 'tx_ring' being != zero and BUG() if that's the case. So after >>> that check there is no way that 'tx_ring' could be anything _but_ zero, so >>> testing it again is just dead code. Once that dead code is removed, the >>> 'pkc' local variable becomes entirely redundant, so remove that as well. >> >> >> What if CONFIG_BUG=n? >> > > Arrgh, I didn't consider that (should have, but didn't).. In that case > we'll have > #define BUG() do {} while(0) > which is not going to work all that peachy with my change... > > David: You may want to pass on this one. I obviously didn't think it > through properly - sorry :-( > It's something I've never been entirely clear on. How are you meant to handle something which really is a fatal bug if CONFIG_BUG=n? I think there are several places in the kernel where the expectation is that BUG() causes a panic that probably don't behave nicely at all (though I guess that might be the expected behaviour) if CONFIG_BUG=n. ~Ryan -- 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: Jesper Juhl <jj@chaosbits.net> Date: Tue, 15 Nov 2011 00:37:33 +0100 (CET) > David: You may want to pass on this one. I obviously didn't think it > through properly - sorry :-( It's already in my tree and pushed out to kernel.org, what in the world do you think "applied" means? -- 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: Jesper Juhl <jj@chaosbits.net> Date: Tue, 15 Nov 2011 00:51:57 +0100 (CET) > On Mon, 14 Nov 2011, David Miller wrote: > >> From: Jesper Juhl <jj@chaosbits.net> >> Date: Tue, 15 Nov 2011 00:37:33 +0100 (CET) >> >> > David: You may want to pass on this one. I obviously didn't think it >> > through properly - sorry :-( >> >> It's already in my tree and pushed out to kernel.org, what in the >> world do you think "applied" means? >> > I'm sorry about that. I try to be careful with my patches, but sometimes > mistakes slip through - I'm only human... I do my best, but I mess up > sometimes and Ryan raised a good point that I had not considered when I > prepared the patch - what would you have me do except reply to his mail as > I did? Send a patch to fix things back up. -- 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 Tue, 15 Nov 2011, Ryan Mallon wrote: > On 15/11/11 10:37, Jesper Juhl wrote: > > > On Mon, 14 Nov 2011, Ryan Mallon wrote: > > > >> On 14/11/11 08:55, Jesper Juhl wrote: > >> > >>> We test for 'tx_ring' being != zero and BUG() if that's the case. So after > >>> that check there is no way that 'tx_ring' could be anything _but_ zero, so > >>> testing it again is just dead code. Once that dead code is removed, the > >>> 'pkc' local variable becomes entirely redundant, so remove that as well. > >> > >> > >> What if CONFIG_BUG=n? > >> > > > > Arrgh, I didn't consider that (should have, but didn't).. In that case > > we'll have > > #define BUG() do {} while(0) > > which is not going to work all that peachy with my change... > > > > David: You may want to pass on this one. I obviously didn't think it > > through properly - sorry :-( > > > > > It's something I've never been entirely clear on. How are you meant to > handle something which really is a fatal bug if CONFIG_BUG=n? > > I think there are several places in the kernel where the expectation is > that BUG() causes a panic that probably don't behave nicely at all > (though I guess that might be the expected behaviour) if CONFIG_BUG=n. > You have a point. Perhaps the proper solution would be to remove CONFIG_BUG so that it is always enabled...? As you say, if something is a fatal bug there is no sane way to continue, so why do we even allow people/systems to try??? That way lies madness it would seem...
On Mon, 14 Nov 2011, David Miller wrote: > From: Jesper Juhl <jj@chaosbits.net> > Date: Tue, 15 Nov 2011 00:37:33 +0100 (CET) > > > David: You may want to pass on this one. I obviously didn't think it > > through properly - sorry :-( > > It's already in my tree and pushed out to kernel.org, what in the > world do you think "applied" means? > I'm sorry about that. I try to be careful with my patches, but sometimes mistakes slip through - I'm only human... I do my best, but I mess up sometimes and Ryan raised a good point that I had not considered when I prepared the patch - what would you have me do except reply to his mail as I did?
On 15/11/11 10:48, Jesper Juhl wrote: > On Tue, 15 Nov 2011, Ryan Mallon wrote: > >> On 15/11/11 10:37, Jesper Juhl wrote: >> >>> On Mon, 14 Nov 2011, Ryan Mallon wrote: >>> >>>> On 14/11/11 08:55, Jesper Juhl wrote: >>>> >>>>> We test for 'tx_ring' being != zero and BUG() if that's the case. So after >>>>> that check there is no way that 'tx_ring' could be anything _but_ zero, so >>>>> testing it again is just dead code. Once that dead code is removed, the >>>>> 'pkc' local variable becomes entirely redundant, so remove that as well. >>>> >>>> What if CONFIG_BUG=n? >>>> >>> Arrgh, I didn't consider that (should have, but didn't).. In that case >>> we'll have >>> #define BUG() do {} while(0) >>> which is not going to work all that peachy with my change... >>> >>> David: You may want to pass on this one. I obviously didn't think it >>> through properly - sorry :-( >>> >> >> It's something I've never been entirely clear on. How are you meant to >> handle something which really is a fatal bug if CONFIG_BUG=n? >> >> I think there are several places in the kernel where the expectation is >> that BUG() causes a panic that probably don't behave nicely at all >> (though I guess that might be the expected behaviour) if CONFIG_BUG=n. >> > You have a point. Perhaps the proper solution would be to remove > CONFIG_BUG so that it is always enabled...? As you say, if something is a > fatal bug there is no sane way to continue, so why do we even allow > people/systems to try??? > That way lies madness it would seem... > The Kconfig looks like this: config BUG bool "BUG() support" if EXPERT default y help Disabling this option eliminates support for BUG and WARN, reducing the size of your kernel image and potentially quietly ignoring numerous fatal conditions. You should only consider disabling this option for embedded systems with no facilities for reporting errors. Just say Y. So you can only disable it if you have CONFIG_EXPERT (read embedded I think). It says that you should only disable it for embedded systems where you have no error reporting facilities, but you probably still want the system to hang/die though since you might have a watchdog which can reset the machine if it hangs. With CONFIG_BUG=n you could potentially hit a BUG() and manage to carry on for sometime even though the system is now unstable. Maybe BUG() should at least drop in to an infinite loop in the case of CONFIG_BUG=n. I might try and come up with a patch later tonight. ~Ryan -- 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
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 82a6f34..ab10e84 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -516,13 +516,11 @@ static void prb_init_blk_timer(struct packet_sock *po, static void prb_setup_retire_blk_timer(struct packet_sock *po, int tx_ring) { - struct tpacket_kbdq_core *pkc; - if (tx_ring) BUG(); - pkc = tx_ring ? &po->tx_ring.prb_bdqc : &po->rx_ring.prb_bdqc; - prb_init_blk_timer(po, pkc, prb_retire_rx_blk_timer_expired); + prb_init_blk_timer(po, &po->rx_ring.prb_bdqc, + prb_retire_rx_blk_timer_expired); } static int prb_calc_retire_blk_tmo(struct packet_sock *po,
We test for 'tx_ring' being != zero and BUG() if that's the case. So after that check there is no way that 'tx_ring' could be anything _but_ zero, so testing it again is just dead code. Once that dead code is removed, the 'pkc' local variable becomes entirely redundant, so remove that as well. Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- net/packet/af_packet.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) only compile tested.