2002-03-21 19:39:08

by Bob Miller

[permalink] [raw]
Subject: [PATCH] 2.5.7 acct.c oops

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.537 -> 1.538
# kernel/acct.c 1.8 -> 1.9
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/03/21 [email protected] 1.538
# Fixed acct.c code by removing the BUG_ON code because it doesn't work
# on UP systems.
# --------------------------------------------
#
diff -Nru a/kernel/acct.c b/kernel/acct.c
--- a/kernel/acct.c Thu Mar 21 11:32:05 2002
+++ b/kernel/acct.c Thu Mar 21 11:32:05 2002
@@ -166,8 +166,6 @@
{
struct file *old_acct = NULL;

- BUG_ON(!spin_is_locked(&acct_globals.lock));
-
if (acct_globals.file) {
old_acct = acct_globals.file;
del_timer(&acct_globals.timer);


2002-03-21 21:28:37

by Bill Davidsen

[permalink] [raw]
Subject: Re: [PATCH] 2.5.7 acct.c oops

On Thu, 21 Mar 2002, Bob Miller wrote:

> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------
> # 02/03/21 [email protected] 1.538
> # Fixed acct.c code by removing the BUG_ON code because it doesn't work
> # on UP systems.
> # --------------------------------------------

Please help my education... after looking at the code, I don't see why
the BUG_ON was removed rather than made dependent on SMP, assuming that
the BK comment and my hasty reading of code actually mean that it did work
for SMP.

Is this a solid "can't happen" now and no test needed, or is a better
test in the works, or ???

I didn't try to compile it, so there may be something I'm totally
missing.

--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

2002-03-21 21:35:58

by Robert Love

[permalink] [raw]
Subject: Re: [PATCH] 2.5.7 acct.c oops

On Thu, 2002-03-21 at 16:26, Bill Davidsen wrote:

> Please help my education... after looking at the code, I don't see why
> the BUG_ON was removed rather than made dependent on SMP, assuming that
> the BK comment and my hasty reading of code actually mean that it did work
> for SMP.
>
> Is this a solid "can't happen" now and no test needed, or is a better
> test in the works, or ???
>
> I didn't try to compile it, so there may be something I'm totally
> missing.

While he could of wrapped the test dependent in #ifdef/#endif
CONFIG_SMP, the test really is not overly needed. It is more a result
of the previous code, which Bob Miller himself fixed up and then much
rewrote the locking for.

Since he recently did the cleanup (and even added that BUG_ON) I trust
he knows if we can remove it.

Robert Love

2002-03-21 22:02:13

by Bob Miller

[permalink] [raw]
Subject: Re: [PATCH] 2.5.7 acct.c oops

On Thu, Mar 21, 2002 at 04:34:49PM -0500, Robert Love wrote:
> On Thu, 2002-03-21 at 16:26, Bill Davidsen wrote:
>
> > Please help my education... after looking at the code, I don't see why
> > the BUG_ON was removed rather than made dependent on SMP, assuming that
> > the BK comment and my hasty reading of code actually mean that it did work
> > for SMP.
> >
> > Is this a solid "can't happen" now and no test needed, or is a better
> > test in the works, or ???
> >
> > I didn't try to compile it, so there may be something I'm totally
> > missing.
>
> While he could of wrapped the test dependent in #ifdef/#endif
> CONFIG_SMP, the test really is not overly needed. It is more a result
> of the previous code, which Bob Miller himself fixed up and then much
> rewrote the locking for.
>
> Since he recently did the cleanup (and even added that BUG_ON) I trust
> he knows if we can remove it.
>
> Robert Love

As Robert has sermized... When I did the cleanup I debated whether to put
the BUG_ON() in there in the first place. I used it as an extra bit of
paranoia not knowing that it would not return correct results on a UP.

--
Bob Miller Email: [email protected]
Open Source Development Lab Phone: 503.626.2455 Ext. 17

2002-03-22 15:28:16

by Bill Davidsen

[permalink] [raw]
Subject: Re: [PATCH] 2.5.7 acct.c oops

On Thu, 21 Mar 2002, Bob Miller wrote:

> On Thu, Mar 21, 2002 at 04:34:49PM -0500, Robert Love wrote:

> > While he could of wrapped the test dependent in #ifdef/#endif
> > CONFIG_SMP, the test really is not overly needed. It is more a result
> > of the previous code, which Bob Miller himself fixed up and then much
> > rewrote the locking for.
> >
> > Since he recently did the cleanup (and even added that BUG_ON) I trust
> > he knows if we can remove it.
> >
> > Robert Love
>
> As Robert has sermized... When I did the cleanup I debated whether to put
> the BUG_ON() in there in the first place. I used it as an extra bit of
> paranoia not knowing that it would not return correct results on a UP.

Okay, that's a reasonable decision. I'm all in favor of paranoia, any
problem at this point would be best detected early on, nut if you're
convinced that it's safe without it, I certainly can't argue, and wasn't
intending to initially.

Actually I don't think CONFIG_SMP would completely avoid problems, if I
read the code right it would still have a problem if you had an SMP kernel
running "nosmp" option at boot (or possibly booted on a uni, although I
haven't tried w/o 'nosmp').

Thanks for all the information, clearly if any check is needed, that
check is not the one, at least as is.

--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.