On Mon, 02 Dec 2013, Dan Carpenter wrote:
> I think the patch is generally ok. It needs to be split up and sent
> through the individual maintainers. The changelog has a lot of extra
> verbiage.
>
> On Mon, Dec 02, 2013 at 03:56:46PM +0100, Nicholas Mc Guire wrote:
> > >From 5b3d0b88438846cbdd888798c2fb04e20c69e95d Mon Sep 17 00:00:00 2001
> > From: Nicholas Mc Guire <der.herr@xxxxxxx>
> > Date: Sun, 1 Dec 2013 22:53:18 -0500
> > Subject: [PATCH] consolidate spin_lock/unlock waiting with spin_unlock_wait
> >
>
> Don't include these bits in the patch and don't indent the changelog by
> one space (don't indent at all, it should be hard left).
>
will drop it - thought the output from git format-patch would be the right
format - my missunderstanding.
> > Hi !
> >
> > Not sure if this is the right place to put this...
> > This patch proposes a small API consolidation.
> >
> > in the following files there is a spin_lock/unlock in use for waiting on
> > some possibly concurrent thread - that is there is a
> >
> > spin_lock(lock);
> > spin_unlock(lock);
> >
> > so in the contended case the calling thread will wait for any concurrent
> > access.
> >
> > arch/blackfin/mach-bf561/smp.c lock/unlock at lines 72 73
> > arch/arm/mach-sti/platsmp.c lock/unlock at lines 53 54
> > arch/cris/arch-v32/drivers/mach-fs/gpio.c lock/unlock at lines 764 765
> > drivers/staging/lustre/lustre/obdclass/llog_obd.c lock/unlock at lines 87 89
> > drivers/net/ethernet/chelsio/cxgb/cxgb2.c lock/unlock at lines 287 288
> > drivers/net/ethernet/ti/cpmac.c lock/unlock at lines 582 583
> > fs/fscache/object.c lock/unlock at lines 692 693
>
> This block is not useful.
>
ok
> >
> > in
> > commit c2f21ce2e31286a0a32f8da0a7856e9ca1122ef3
> > Author: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Date: Wed Dec 2 20:02:59 2009 +0100
> >
> > locking: Implement new raw_spinlock
> >
>
> This is not the patch which introduced spin_unlock_wait(). That
> function was around before we adopted git.
>
used git blame to find that - how would I find this information or is
it not relevant to include it ?
> > the api was extended with include/linux/spinlock.h:spin_unlock_wait() for
> > just this purpose (I think atleast) so the below patch changes these
> > currently hard-coded spin_lock/unlock to use the available API.
> > so as its in spinlock.h and the code in question is using spin_lock/unlock
> > no new header file inclusion is needed.
>
> We hopefully can assume you compile tested these so this information
> about header files is redundant. If you didn't compile test, then we
> will get really annoyed.
>
yes but only ran it on a 32 and 64 bit box to test them - but taht would
at most cover the case in fs so that is very limited. What I did is generate
the .i files and then looked at the function that got plugged in at that line
e.g. arch_spin_unlock_wait in the fs/fscache/object.c then checked if that
should be equivalent. in the arch/cris/arch-v32/drivers/mach-fs/gpio.c
I contacted the maintainer of the file as I could not figure out what it was
waiting on (and its UP only).
> >
> > all the above listed cases are consecutive spin_lock/spin_unlock.
> > the one-line offset in llog_obd.c at lines 87 89 is due to a comment not code
> > so it also is a consecutive spin_lock/spin_unlock,
>
> Obvious. Don't include this in the commit message.
>
ok
> >
> > If these changes make any sense should this go out to all of the code authors
> > for review/ack or is this simple enough for it to just go out via
> > kernel-janitors/LKML for review ?
> >
> > patch is against 3.13.0-rc1 (linux-stable Greg KH tree)
> >
>
> Don't do patches against stable. Do them against the current -rc or
> against linux-next. In theory, the current -rc is better because then
> you can test them without other patches affecting the results, but in
> real life, you don't have the hardware for most of these and I doubt you
> are going to test the filesystem ones.
>
ok - will redo it again linux-next and send it out to the maintainers for review
> > @@ -284,8 +284,7 @@ static int cxgb_close(struct net_device *dev)
> > !(adapter->open_device_map & PORT_MASK)) {
> > /* Stop statistics accumulation. */
> > smp_mb__after_clear_bit();
> > - spin_lock(&adapter->work_lock); /* sync with update task */
> ^^^^^^^^^^^^^^^^^^^^^
> Keep this comment. It's useful.
>
thanks - comletely overlooked that I was removing information not just code
> > - spin_unlock(&adapter->work_lock);
> > + spin_unlock_wait(&adapter->work_lock);
> > cancel_mac_stats_update(adapter);
> > }
> >
>
thanks! will fixup split it and send it to the file maintainers.
thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
