[PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Jesper Juhl]

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.

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,
-- 
1.7.7.3


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[David Miller]

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.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Ryan Mallon]

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,

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[chetan loke]

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

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Jesper Juhl]

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 :-(

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Ryan Mallon]

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

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[David Miller]

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?

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[David Miller]

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.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Jesper Juhl]

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...


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Jesper Juhl]

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?

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Ryan Mallon]

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

Re: [PATCH] net/packet: remove dead code and unneeded variable from prb_setup_retire_blk_timer()

[Jesper Juhl]

On Mon, 14 Nov 2011, David Miller wrote:

> 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.

Sure, I can do that. Please see below.

I'm sorry about thiss mess and the extra work it caused you - I'll be more 
careful in the future.


From: Jesper Juhl <jj@chaosbits.net>
Subject: [PATCH] Revert incorrect dead-code changes to prb_setup_retire_blk_timer

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 net/packet/af_packet.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index ab10e84..82a6f34 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -516,11 +516,13 @@ 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();
 
-	prb_init_blk_timer(po, &po->rx_ring.prb_bdqc,
-			   prb_retire_rx_blk_timer_expired);
+	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);
 }
 
 static int prb_calc_retire_blk_tmo(struct packet_sock *po,
-- 
1.7.7.3

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.