Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/10/13 17:34), Sergey Senozhatsky wrote: [..] > > > > Now I think we can drop the call to handle_pending_slot_free() in > > zram_bvec_rw() altogether. As long as the write lock is held when > > handle_pending_slot_free() is called, there is no race. It's no different > > from any write request and the current code handles R/W concurrency > > already. > > Yes, I think that can work. > > To summarize, there should be 3 patches: > 1) handle_pending_slot_free() in zram_bvec_rw() (as suggested by Jerome > Marchand) > 2) handle_pending_slot_free() race with reset (found by Dan Carpenter) > 3) drop init_done and use init_done() > > I'll prepare a patches later today. I've sent two patches: staging: zram: fix handle_pending_slot_free() and zram_reset_device() race staging: zram: remove init_done from zram struct (v3) Cc'd driverdev-de...@linuxdriverproject.org as suggested by Dan. please discard any previous patches and sorry for the noise. Thanks, -ss > > > Jerome > > > > > > > >> > > >> 1) You haven't given us any performance numbers so it's not clear if the > > >>locking is even a problem. > > >> > > >> 2) The v2 patch introduces an obvious deadlock in zram_slot_free() > > >>because now we take the rw_lock twice. Fix your testing to catch > > >>this kind of bug next time. > > >> > > >> 3) Explain why it is safe to test zram->slot_free_rq when we are not > > >>holding the lock. I think it is unsafe. I don't want to even think > > >>about it without the numbers. > > >> > > >> regards, > > >> dan carpenter > > >> > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > > the body of a message to majord...@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Tue, Sep 10, 2013 at 05:58:02PM +0300, Dan Carpenter wrote: > Btw, the de...@driverdev.osuosl.org list seems to be down again. I > still have not recieved the v3 patch. Use the > driverdev-de...@linuxdriverproject.org email list instead. They are the same "list", just different DNS entries. I'll go poke the sysadmin to find out what's going on... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
Btw, the de...@driverdev.osuosl.org list seems to be down again. I still have not recieved the v3 patch. Use the driverdev-de...@linuxdriverproject.org email list instead. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 18:10), Jerome Marchand wrote: > On 09/09/2013 03:46 PM, Jerome Marchand wrote: > > On 09/09/2013 03:21 PM, Dan Carpenter wrote: > >> On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: > > Calling handle_pending_slot_free() for every RW operation may > > cause unneccessary slot_free_lock locking, because most likely > > process will see NULL slot_free_rq. handle_pending_slot_free() > > only when current detects that slot_free_rq is not NULL. > > > > v2: protect handle_pending_slot_free() with zram rw_lock. > > > > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram > rw_lock be wrapped around the whole operation like the original code > does? I don't know the zram code, but the original looks like it makes > sense but in this one it looks like the locks are duplicative. > > Is the down_read() in the original code be changed to down_write()? > > >>> > >>> I'm not touching locking around existing READ/WRITE commands. > >>> > >> > >> Your patch does change the locking because now instead of taking the > >> zram lock once it takes it and then drops it and then retakes it. This > >> looks potentially racy to me but I don't know the code so I will defer > >> to any zram maintainer. > > > > You're right. Nothing prevents zram_slot_free_notify() to repopulate the > > free slot queue while we drop the lock. > > > > Actually, the original code is already racy. handle_pending_slot_free() > > modifies zram->table while holding only a read lock. It needs to hold a > > write lock to do that. Using down_write for all requests would obviously > > fix that, but at the cost of read performance. > > Now I think we can drop the call to handle_pending_slot_free() in > zram_bvec_rw() altogether. As long as the write lock is held when > handle_pending_slot_free() is called, there is no race. It's no different > from any write request and the current code handles R/W concurrency > already. Yes, I think that can work. To summarize, there should be 3 patches: 1) handle_pending_slot_free() in zram_bvec_rw() (as suggested by Jerome Marchand) 2) handle_pending_slot_free() race with reset (found by Dan Carpenter) 3) drop init_done and use init_done() I'll prepare a patches later today. -ss > Jerome > > > > >> > >> 1) You haven't given us any performance numbers so it's not clear if the > >>locking is even a problem. > >> > >> 2) The v2 patch introduces an obvious deadlock in zram_slot_free() > >>because now we take the rw_lock twice. Fix your testing to catch > >>this kind of bug next time. > >> > >> 3) Explain why it is safe to test zram->slot_free_rq when we are not > >>holding the lock. I think it is unsafe. I don't want to even think > >>about it without the numbers. > >> > >> regards, > >> dan carpenter > >> > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 18:10), Jerome Marchand wrote: On 09/09/2013 03:46 PM, Jerome Marchand wrote: On 09/09/2013 03:21 PM, Dan Carpenter wrote: On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? I'm not touching locking around existing READ/WRITE commands. Your patch does change the locking because now instead of taking the zram lock once it takes it and then drops it and then retakes it. This looks potentially racy to me but I don't know the code so I will defer to any zram maintainer. You're right. Nothing prevents zram_slot_free_notify() to repopulate the free slot queue while we drop the lock. Actually, the original code is already racy. handle_pending_slot_free() modifies zram-table while holding only a read lock. It needs to hold a write lock to do that. Using down_write for all requests would obviously fix that, but at the cost of read performance. Now I think we can drop the call to handle_pending_slot_free() in zram_bvec_rw() altogether. As long as the write lock is held when handle_pending_slot_free() is called, there is no race. It's no different from any write request and the current code handles R/W concurrency already. Yes, I think that can work. To summarize, there should be 3 patches: 1) handle_pending_slot_free() in zram_bvec_rw() (as suggested by Jerome Marchand) 2) handle_pending_slot_free() race with reset (found by Dan Carpenter) 3) drop init_done and use init_done() I'll prepare a patches later today. -ss Jerome 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
Btw, the de...@driverdev.osuosl.org list seems to be down again. I still have not recieved the v3 patch. Use the driverdev-de...@linuxdriverproject.org email list instead. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Tue, Sep 10, 2013 at 05:58:02PM +0300, Dan Carpenter wrote: Btw, the de...@driverdev.osuosl.org list seems to be down again. I still have not recieved the v3 patch. Use the driverdev-de...@linuxdriverproject.org email list instead. They are the same list, just different DNS entries. I'll go poke the sysadmin to find out what's going on... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/10/13 17:34), Sergey Senozhatsky wrote: [..] Now I think we can drop the call to handle_pending_slot_free() in zram_bvec_rw() altogether. As long as the write lock is held when handle_pending_slot_free() is called, there is no race. It's no different from any write request and the current code handles R/W concurrency already. Yes, I think that can work. To summarize, there should be 3 patches: 1) handle_pending_slot_free() in zram_bvec_rw() (as suggested by Jerome Marchand) 2) handle_pending_slot_free() race with reset (found by Dan Carpenter) 3) drop init_done and use init_done() I'll prepare a patches later today. I've sent two patches: staging: zram: fix handle_pending_slot_free() and zram_reset_device() race staging: zram: remove init_done from zram struct (v3) Cc'd driverdev-de...@linuxdriverproject.org as suggested by Dan. please discard any previous patches and sorry for the noise. Thanks, -ss Jerome 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On 09/09/2013 03:46 PM, Jerome Marchand wrote: > On 09/09/2013 03:21 PM, Dan Carpenter wrote: >> On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: > Calling handle_pending_slot_free() for every RW operation may > cause unneccessary slot_free_lock locking, because most likely > process will see NULL slot_free_rq. handle_pending_slot_free() > only when current detects that slot_free_rq is not NULL. > > v2: protect handle_pending_slot_free() with zram rw_lock. > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? >>> >>> I'm not touching locking around existing READ/WRITE commands. >>> >> >> Your patch does change the locking because now instead of taking the >> zram lock once it takes it and then drops it and then retakes it. This >> looks potentially racy to me but I don't know the code so I will defer >> to any zram maintainer. > > You're right. Nothing prevents zram_slot_free_notify() to repopulate the > free slot queue while we drop the lock. > > Actually, the original code is already racy. handle_pending_slot_free() > modifies zram->table while holding only a read lock. It needs to hold a > write lock to do that. Using down_write for all requests would obviously > fix that, but at the cost of read performance. Now I think we can drop the call to handle_pending_slot_free() in zram_bvec_rw() altogether. As long as the write lock is held when handle_pending_slot_free() is called, there is no race. It's no different from any write request and the current code handles R/W concurrency already. Jerome > >> >> 1) You haven't given us any performance numbers so it's not clear if the >>locking is even a problem. >> >> 2) The v2 patch introduces an obvious deadlock in zram_slot_free() >>because now we take the rw_lock twice. Fix your testing to catch >>this kind of bug next time. >> >> 3) Explain why it is safe to test zram->slot_free_rq when we are not >>holding the lock. I think it is unsafe. I don't want to even think >>about it without the numbers. >> >> regards, >> dan carpenter >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 17:52), Dan Carpenter wrote: > On Mon, Sep 09, 2013 at 05:42:59PM +0300, Sergey Senozhatsky wrote: > > > 3) Explain why it is safe to test zram->slot_free_rq when we are not > > >holding the lock. I think it is unsafe. I don't want to even think > > >about it without the numbers. > > > > atomic pointer test, which is either NULL or !NULL. > > > > That's not how concurency works. Atomic types are complicated than > that. Anyway, the zram maintainers don't need me to explain that to > them so I'll let them take over from here. > yes, I understand that. but can't we check slot_free_rq pointer (32 or 64 bit read) w/o locking to just decide if we must: -- call handle_pending_slot_free() -- take the slot_free_lock -- check slot_free_rq again (this time under the slot_free_lock) and perform slot_free_rq operations while it is !NULL. -ss > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Mon, Sep 09, 2013 at 05:42:59PM +0300, Sergey Senozhatsky wrote: > > 3) Explain why it is safe to test zram->slot_free_rq when we are not > >holding the lock. I think it is unsafe. I don't want to even think > >about it without the numbers. > > atomic pointer test, which is either NULL or !NULL. > That's not how concurency works. Atomic types are complicated than that. Anyway, the zram maintainers don't need me to explain that to them so I'll let them take over from here. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 16:21), Dan Carpenter wrote:
> On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote:
> > > > Calling handle_pending_slot_free() for every RW operation may
> > > > cause unneccessary slot_free_lock locking, because most likely
> > > > process will see NULL slot_free_rq. handle_pending_slot_free()
> > > > only when current detects that slot_free_rq is not NULL.
> > > >
> > > > v2: protect handle_pending_slot_free() with zram rw_lock.
> > > >
> > >
> > > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram
> > > rw_lock be wrapped around the whole operation like the original code
> > > does? I don't know the zram code, but the original looks like it makes
> > > sense but in this one it looks like the locks are duplicative.
> > >
> > > Is the down_read() in the original code be changed to down_write()?
> > >
> >
> > I'm not touching locking around existing READ/WRITE commands.
> >
>
> Your patch does change the locking because now instead of taking the
> zram lock once it takes it and then drops it and then retakes it. This
> looks potentially racy to me but I don't know the code so I will defer
> to any zram maintainer.
>
it takes it only when there is zram->slot_free_rq. otherwise it just
pointer comparison. original code does (schematically for READ case)
down_read()
spin_lock()
if (!NULL) {...}
spin_unlock();
up_read();
patch adds the `if (!NULL)' check before N concurrent readers will
stuck on spin_lock() to just unlock spin_lock because zram->slot_free_rq
is NULL.
> 1) You haven't given us any performance numbers so it's not clear if the
>locking is even a problem.
good point, I did not perform any testing. here they are:
./iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
Record Size 16 KB
File size set to 61440 KB
O_DIRECT feature enabled
Command line used: ./iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
Output is in Kbytes/sec
Time Resolution = 0.01 seconds.
Processor cache size set to 1024 Kbytes.
Processor cache line size set to 32 bytes.
File stride size set to 17 * record size.
Min process = 3
Max process = 3
Throughput test with 3 processes
Each process writes a 61440 Kbyte file in 16 Kbyte records
test |w/o patch |w/ patch
--
Read | 1895278.31 | 2778439.78
Re-read | 2638231.06 | 2630383.88
Reverse Read | 1378348.80 | 1538697.89
Stride read | 1698457.96 | 2043402.42
Random read | 1818998.33 | 2038670.38
Mixed workload |376585.98 |435064.57
Pread | 1402478.04 |992575.22
Fread | 4955286.31 | 5199061.31
> 2) The v2 patch introduces an obvious deadlock in zram_slot_free()
>because now we take the rw_lock twice. Fix your testing to catch
>this kind of bug next time.
yes it is. and I'm sorry about that. I sent v3 yesterday
https://lkml.org/lkml/2013/9/8/42
> 3) Explain why it is safe to test zram->slot_free_rq when we are not
>holding the lock. I think it is unsafe. I don't want to even think
>about it without the numbers.
atomic pointer test, which is either NULL or !NULL.
for NULL case we don't take spin lock and just skip the whole
handle_pending_slot_free() thing.
for !NULL we call handle_pending_slot_free(). any operations with
zram->slot_free_rq
(walking, removal or addition of element) are protected by spin lock within
handle_pending_slot_free(). most requests will see NULL zram->slot_free_rq.
if someone set zram->slot_free_rq to !NULL right after current process checked
it, then next request will see it !NULL and perform handle_pending_slot_free().
-ss
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On 09/09/2013 03:21 PM, Dan Carpenter wrote: > On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. >>> >>> zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram >>> rw_lock be wrapped around the whole operation like the original code >>> does? I don't know the zram code, but the original looks like it makes >>> sense but in this one it looks like the locks are duplicative. >>> >>> Is the down_read() in the original code be changed to down_write()? >>> >> >> I'm not touching locking around existing READ/WRITE commands. >> > > Your patch does change the locking because now instead of taking the > zram lock once it takes it and then drops it and then retakes it. This > looks potentially racy to me but I don't know the code so I will defer > to any zram maintainer. You're right. Nothing prevents zram_slot_free_notify() to repopulate the free slot queue while we drop the lock. Actually, the original code is already racy. handle_pending_slot_free() modifies zram->table while holding only a read lock. It needs to hold a write lock to do that. Using down_write for all requests would obviously fix that, but at the cost of read performance. > > 1) You haven't given us any performance numbers so it's not clear if the >locking is even a problem. > > 2) The v2 patch introduces an obvious deadlock in zram_slot_free() >because now we take the rw_lock twice. Fix your testing to catch >this kind of bug next time. > > 3) Explain why it is safe to test zram->slot_free_rq when we are not >holding the lock. I think it is unsafe. I don't want to even think >about it without the numbers. > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: > > > Calling handle_pending_slot_free() for every RW operation may > > > cause unneccessary slot_free_lock locking, because most likely > > > process will see NULL slot_free_rq. handle_pending_slot_free() > > > only when current detects that slot_free_rq is not NULL. > > > > > > v2: protect handle_pending_slot_free() with zram rw_lock. > > > > > > > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram > > rw_lock be wrapped around the whole operation like the original code > > does? I don't know the zram code, but the original looks like it makes > > sense but in this one it looks like the locks are duplicative. > > > > Is the down_read() in the original code be changed to down_write()? > > > > I'm not touching locking around existing READ/WRITE commands. > Your patch does change the locking because now instead of taking the zram lock once it takes it and then drops it and then retakes it. This looks potentially racy to me but I don't know the code so I will defer to any zram maintainer. 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram->slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
> > Calling handle_pending_slot_free() for every RW operation may
> > cause unneccessary slot_free_lock locking, because most likely
> > process will see NULL slot_free_rq. handle_pending_slot_free()
> > only when current detects that slot_free_rq is not NULL.
> >
> > v2: protect handle_pending_slot_free() with zram rw_lock.
> >
>
> zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram
> rw_lock be wrapped around the whole operation like the original code
> does? I don't know the zram code, but the original looks like it makes
> sense but in this one it looks like the locks are duplicative.
>
> Is the down_read() in the original code be changed to down_write()?
>
I'm not touching locking around existing READ/WRITE commands.
the original code:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
spin_lock(>slot_free_lock);
while (zram->slot_free_rq) {
free_rq = zram->slot_free_rq;
zram->slot_free_rq = free_rq->next;
zram_free_page(zram, free_rq->index);
kfree(free_rq);
}
spin_unlock(>slot_free_lock);
}
static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
int offset, struct bio *bio, int rw)
{
int ret;
if (rw == READ) {
down_read(>lock);
handle_pending_slot_free(zram);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(>lock);
} else {
down_write(>lock);
handle_pending_slot_free(zram);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(>lock);
}
return ret;
}
the new one:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
down_write(>lock);
spin_lock(>slot_free_lock);
while (zram->slot_free_rq) {
free_rq = zram->slot_free_rq;
zram->slot_free_rq = free_rq->next;
zram_free_page(zram, free_rq->index);
kfree(free_rq);
}
spin_unlock(>slot_free_lock);
up_write(>lock);
}
static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
int offset, struct bio *bio, int rw)
{
int ret;
if (zram->slot_free_rq)
handle_pending_slot_free(zram);
if (rw == READ) {
down_read(>lock);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(>lock);
} else {
down_write(>lock);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(>lock);
}
return ret;
}
both READ and WRITE operations are still protected by down_read() for READ path
and down_write() for WRITE path. however, there is no handle_pending_slot_free()
and zram->slot_free_lock locking on every READ/WRITE, instead
handle_pending_slot_free()
is called only when zram->slot_free_rq is not NULL. handle_pending_slot_free()
in
turn protects zram_free_page() call by down_write(), so no READ/WRITE operations
are affected.
-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Fri, Sep 06, 2013 at 06:12:55PM +0300, Sergey Senozhatsky wrote: > Calling handle_pending_slot_free() for every RW operation may > cause unneccessary slot_free_lock locking, because most likely > process will see NULL slot_free_rq. handle_pending_slot_free() > only when current detects that slot_free_rq is not NULL. > > v2: protect handle_pending_slot_free() with zram rw_lock. > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On (09/09/13 11:33), Dan Carpenter wrote:
> On Fri, Sep 06, 2013 at 05:55:45PM +0300, Sergey Senozhatsky wrote:
> > On (09/06/13 16:42), Jerome Marchand wrote:
> > > On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
> > > > Calling handle_pending_slot_free() for every RW operation may
> > > > cause unneccessary slot_free_lock locking, because most likely
> > > > process will see NULL slot_free_rq. handle_pending_slot_free()
> > > > only when current detects that slot_free_rq is not NULL.
> > > >
> > > > Signed-off-by: Sergey Senozhatsky
> > > >
> > > > ---
> > > >
> > > > drivers/staging/zram/zram_drv.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/zram/zram_drv.c
> > > > b/drivers/staging/zram/zram_drv.c
> > > > index 91d94b5..17386e2 100644
> > > > --- a/drivers/staging/zram/zram_drv.c
> > > > +++ b/drivers/staging/zram/zram_drv.c
> > > > @@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
> > > > bio_vec *bvec, u32 index,
> > > > {
> > > > int ret;
> > > >
> > > > + if (zram->slot_free_rq)
> > > > + handle_pending_slot_free(zram);
> > > > +
> > >
> > > Calling handle_pending_slot_free() without holding zram->lock?
> > > That's racy.
> >
> > sorry, my bad. it should take down_write() lock.
> >
>
> Or down_read() on the read path. We leave the original as-is?
>
Hello,
down_write() for both READ and WRITE looks ok to me (+down_write()
for zram_slot_free()). is there something I miss?
down_read() for READ in case of N active readers will force N-1
processes to spin on zram->slot_free_lock in handle_pending_slot_free().
it probably makes sense to add extra zram->slot_free_rq check for
the case when process slept on rw lock while someone was freeing
pages:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
down_write(>lock);
+ if (!zram->slot_free_rq)
+ goto out;
spin_lock(>slot_free_lock);
while (zram->slot_free_rq) {
free_rq = zram->slot_free_rq;
zram->slot_free_rq = free_rq->next;
zram_free_page(zram, free_rq->index);
kfree(free_rq);
}
spin_unlock(>slot_free_lock);
+out:
up_write(>lock);
}
-ss
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On Fri, Sep 06, 2013 at 05:55:45PM +0300, Sergey Senozhatsky wrote:
> On (09/06/13 16:42), Jerome Marchand wrote:
> > On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
> > > Calling handle_pending_slot_free() for every RW operation may
> > > cause unneccessary slot_free_lock locking, because most likely
> > > process will see NULL slot_free_rq. handle_pending_slot_free()
> > > only when current detects that slot_free_rq is not NULL.
> > >
> > > Signed-off-by: Sergey Senozhatsky
> > >
> > > ---
> > >
> > > drivers/staging/zram/zram_drv.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/zram/zram_drv.c
> > > b/drivers/staging/zram/zram_drv.c
> > > index 91d94b5..17386e2 100644
> > > --- a/drivers/staging/zram/zram_drv.c
> > > +++ b/drivers/staging/zram/zram_drv.c
> > > @@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
> > > bio_vec *bvec, u32 index,
> > > {
> > > int ret;
> > >
> > > + if (zram->slot_free_rq)
> > > + handle_pending_slot_free(zram);
> > > +
> >
> > Calling handle_pending_slot_free() without holding zram->lock?
> > That's racy.
>
> sorry, my bad. it should take down_write() lock.
>
Or down_read() on the read path. We leave the original as-is?
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On (09/09/13 11:33), Dan Carpenter wrote:
On Fri, Sep 06, 2013 at 05:55:45PM +0300, Sergey Senozhatsky wrote:
On (09/06/13 16:42), Jerome Marchand wrote:
On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
---
drivers/staging/zram/zram_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c
b/drivers/staging/zram/zram_drv.c
index 91d94b5..17386e2 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
bio_vec *bvec, u32 index,
{
int ret;
+ if (zram-slot_free_rq)
+ handle_pending_slot_free(zram);
+
Calling handle_pending_slot_free() without holding zram-lock?
That's racy.
sorry, my bad. it should take down_write() lock.
Or down_read() on the read path. We leave the original as-is?
Hello,
down_write() for both READ and WRITE looks ok to me (+down_write()
for zram_slot_free()). is there something I miss?
down_read() for READ in case of N active readers will force N-1
processes to spin on zram-slot_free_lock in handle_pending_slot_free().
it probably makes sense to add extra zram-slot_free_rq check for
the case when process slept on rw lock while someone was freeing
pages:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
down_write(zram-lock);
+ if (!zram-slot_free_rq)
+ goto out;
spin_lock(zram-slot_free_lock);
while (zram-slot_free_rq) {
free_rq = zram-slot_free_rq;
zram-slot_free_rq = free_rq-next;
zram_free_page(zram, free_rq-index);
kfree(free_rq);
}
spin_unlock(zram-slot_free_lock);
+out:
up_write(zram-lock);
}
-ss
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Fri, Sep 06, 2013 at 06:12:55PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
v2: protect handle_pending_slot_free() with zram rw_lock.
zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram
rw_lock be wrapped around the whole operation like the original code
does? I don't know the zram code, but the original looks like it makes
sense but in this one it looks like the locks are duplicative.
Is the down_read() in the original code be changed to down_write()?
I'm not touching locking around existing READ/WRITE commands.
the original code:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
spin_lock(zram-slot_free_lock);
while (zram-slot_free_rq) {
free_rq = zram-slot_free_rq;
zram-slot_free_rq = free_rq-next;
zram_free_page(zram, free_rq-index);
kfree(free_rq);
}
spin_unlock(zram-slot_free_lock);
}
static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
int offset, struct bio *bio, int rw)
{
int ret;
if (rw == READ) {
down_read(zram-lock);
handle_pending_slot_free(zram);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(zram-lock);
} else {
down_write(zram-lock);
handle_pending_slot_free(zram);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(zram-lock);
}
return ret;
}
the new one:
static void handle_pending_slot_free(struct zram *zram)
{
struct zram_slot_free *free_rq;
down_write(zram-lock);
spin_lock(zram-slot_free_lock);
while (zram-slot_free_rq) {
free_rq = zram-slot_free_rq;
zram-slot_free_rq = free_rq-next;
zram_free_page(zram, free_rq-index);
kfree(free_rq);
}
spin_unlock(zram-slot_free_lock);
up_write(zram-lock);
}
static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
int offset, struct bio *bio, int rw)
{
int ret;
if (zram-slot_free_rq)
handle_pending_slot_free(zram);
if (rw == READ) {
down_read(zram-lock);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(zram-lock);
} else {
down_write(zram-lock);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(zram-lock);
}
return ret;
}
both READ and WRITE operations are still protected by down_read() for READ path
and down_write() for WRITE path. however, there is no handle_pending_slot_free()
and zram-slot_free_lock locking on every READ/WRITE, instead
handle_pending_slot_free()
is called only when zram-slot_free_rq is not NULL. handle_pending_slot_free()
in
turn protects zram_free_page() call by down_write(), so no READ/WRITE operations
are affected.
-ss
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? I'm not touching locking around existing READ/WRITE commands. Your patch does change the locking because now instead of taking the zram lock once it takes it and then drops it and then retakes it. This looks potentially racy to me but I don't know the code so I will defer to any zram maintainer. 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On 09/09/2013 03:21 PM, Dan Carpenter wrote: On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? I'm not touching locking around existing READ/WRITE commands. Your patch does change the locking because now instead of taking the zram lock once it takes it and then drops it and then retakes it. This looks potentially racy to me but I don't know the code so I will defer to any zram maintainer. You're right. Nothing prevents zram_slot_free_notify() to repopulate the free slot queue while we drop the lock. Actually, the original code is already racy. handle_pending_slot_free() modifies zram-table while holding only a read lock. It needs to hold a write lock to do that. Using down_write for all requests would obviously fix that, but at the cost of read performance. 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 16:21), Dan Carpenter wrote:
On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote:
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
v2: protect handle_pending_slot_free() with zram rw_lock.
zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram
rw_lock be wrapped around the whole operation like the original code
does? I don't know the zram code, but the original looks like it makes
sense but in this one it looks like the locks are duplicative.
Is the down_read() in the original code be changed to down_write()?
I'm not touching locking around existing READ/WRITE commands.
Your patch does change the locking because now instead of taking the
zram lock once it takes it and then drops it and then retakes it. This
looks potentially racy to me but I don't know the code so I will defer
to any zram maintainer.
it takes it only when there is zram-slot_free_rq. otherwise it just
pointer comparison. original code does (schematically for READ case)
down_read()
spin_lock()
if (!NULL) {...}
spin_unlock();
up_read();
patch adds the `if (!NULL)' check before N concurrent readers will
stuck on spin_lock() to just unlock spin_lock because zram-slot_free_rq
is NULL.
1) You haven't given us any performance numbers so it's not clear if the
locking is even a problem.
good point, I did not perform any testing. here they are:
./iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
Record Size 16 KB
File size set to 61440 KB
O_DIRECT feature enabled
Command line used: ./iozone -t -T -R -l 3 -u 3 -r 16K -s 60M -I +Z
Output is in Kbytes/sec
Time Resolution = 0.01 seconds.
Processor cache size set to 1024 Kbytes.
Processor cache line size set to 32 bytes.
File stride size set to 17 * record size.
Min process = 3
Max process = 3
Throughput test with 3 processes
Each process writes a 61440 Kbyte file in 16 Kbyte records
test |w/o patch |w/ patch
--
Read | 1895278.31 | 2778439.78
Re-read | 2638231.06 | 2630383.88
Reverse Read | 1378348.80 | 1538697.89
Stride read | 1698457.96 | 2043402.42
Random read | 1818998.33 | 2038670.38
Mixed workload |376585.98 |435064.57
Pread | 1402478.04 |992575.22
Fread | 4955286.31 | 5199061.31
2) The v2 patch introduces an obvious deadlock in zram_slot_free()
because now we take the rw_lock twice. Fix your testing to catch
this kind of bug next time.
yes it is. and I'm sorry about that. I sent v3 yesterday
https://lkml.org/lkml/2013/9/8/42
3) Explain why it is safe to test zram-slot_free_rq when we are not
holding the lock. I think it is unsafe. I don't want to even think
about it without the numbers.
atomic pointer test, which is either NULL or !NULL.
for NULL case we don't take spin lock and just skip the whole
handle_pending_slot_free() thing.
for !NULL we call handle_pending_slot_free(). any operations with
zram-slot_free_rq
(walking, removal or addition of element) are protected by spin lock within
handle_pending_slot_free(). most requests will see NULL zram-slot_free_rq.
if someone set zram-slot_free_rq to !NULL right after current process checked
it, then next request will see it !NULL and perform handle_pending_slot_free().
-ss
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On Mon, Sep 09, 2013 at 05:42:59PM +0300, Sergey Senozhatsky wrote: 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. atomic pointer test, which is either NULL or !NULL. That's not how concurency works. Atomic types are complicated than that. Anyway, the zram maintainers don't need me to explain that to them so I'll let them take over from here. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On (09/09/13 17:52), Dan Carpenter wrote: On Mon, Sep 09, 2013 at 05:42:59PM +0300, Sergey Senozhatsky wrote: 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. atomic pointer test, which is either NULL or !NULL. That's not how concurency works. Atomic types are complicated than that. Anyway, the zram maintainers don't need me to explain that to them so I'll let them take over from here. yes, I understand that. but can't we check slot_free_rq pointer (32 or 64 bit read) w/o locking to just decide if we must: -- call handle_pending_slot_free() -- take the slot_free_lock -- check slot_free_rq again (this time under the slot_free_lock) and perform slot_free_rq operations while it is !NULL. -ss regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)
On 09/09/2013 03:46 PM, Jerome Marchand wrote: On 09/09/2013 03:21 PM, Dan Carpenter wrote: On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote: Calling handle_pending_slot_free() for every RW operation may cause unneccessary slot_free_lock locking, because most likely process will see NULL slot_free_rq. handle_pending_slot_free() only when current detects that slot_free_rq is not NULL. v2: protect handle_pending_slot_free() with zram rw_lock. zram-slot_free_lock protects zram-slot_free_rq but shouldn't the zram rw_lock be wrapped around the whole operation like the original code does? I don't know the zram code, but the original looks like it makes sense but in this one it looks like the locks are duplicative. Is the down_read() in the original code be changed to down_write()? I'm not touching locking around existing READ/WRITE commands. Your patch does change the locking because now instead of taking the zram lock once it takes it and then drops it and then retakes it. This looks potentially racy to me but I don't know the code so I will defer to any zram maintainer. You're right. Nothing prevents zram_slot_free_notify() to repopulate the free slot queue while we drop the lock. Actually, the original code is already racy. handle_pending_slot_free() modifies zram-table while holding only a read lock. It needs to hold a write lock to do that. Using down_write for all requests would obviously fix that, but at the cost of read performance. Now I think we can drop the call to handle_pending_slot_free() in zram_bvec_rw() altogether. As long as the write lock is held when handle_pending_slot_free() is called, there is no race. It's no different from any write request and the current code handles R/W concurrency already. Jerome 1) You haven't given us any performance numbers so it's not clear if the locking is even a problem. 2) The v2 patch introduces an obvious deadlock in zram_slot_free() because now we take the rw_lock twice. Fix your testing to catch this kind of bug next time. 3) Explain why it is safe to test zram-slot_free_rq when we are not holding the lock. I think it is unsafe. I don't want to even think about it without the numbers. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On Fri, Sep 06, 2013 at 05:55:45PM +0300, Sergey Senozhatsky wrote:
On (09/06/13 16:42), Jerome Marchand wrote:
On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
---
drivers/staging/zram/zram_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c
b/drivers/staging/zram/zram_drv.c
index 91d94b5..17386e2 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
bio_vec *bvec, u32 index,
{
int ret;
+ if (zram-slot_free_rq)
+ handle_pending_slot_free(zram);
+
Calling handle_pending_slot_free() without holding zram-lock?
That's racy.
sorry, my bad. it should take down_write() lock.
Or down_read() on the read path. We leave the original as-is?
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On (09/06/13 16:42), Jerome Marchand wrote:
> On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
> > Calling handle_pending_slot_free() for every RW operation may
> > cause unneccessary slot_free_lock locking, because most likely
> > process will see NULL slot_free_rq. handle_pending_slot_free()
> > only when current detects that slot_free_rq is not NULL.
> >
> > Signed-off-by: Sergey Senozhatsky
> >
> > ---
> >
> > drivers/staging/zram/zram_drv.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/zram/zram_drv.c
> > b/drivers/staging/zram/zram_drv.c
> > index 91d94b5..17386e2 100644
> > --- a/drivers/staging/zram/zram_drv.c
> > +++ b/drivers/staging/zram/zram_drv.c
> > @@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
> > bio_vec *bvec, u32 index,
> > {
> > int ret;
> >
> > + if (zram->slot_free_rq)
> > + handle_pending_slot_free(zram);
> > +
>
> Calling handle_pending_slot_free() without holding zram->lock?
> That's racy.
sorry, my bad. it should take down_write() lock.
-ss
> Jerome
>
> > if (rw == READ) {
> > down_read(>lock);
> > - handle_pending_slot_free(zram);
> > ret = zram_bvec_read(zram, bvec, index, offset, bio);
> > up_read(>lock);
> > } else {
> > down_write(>lock);
> > - handle_pending_slot_free(zram);
> > ret = zram_bvec_write(zram, bvec, index, offset);
> > up_write(>lock);
> > }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
> Calling handle_pending_slot_free() for every RW operation may
> cause unneccessary slot_free_lock locking, because most likely
> process will see NULL slot_free_rq. handle_pending_slot_free()
> only when current detects that slot_free_rq is not NULL.
>
> Signed-off-by: Sergey Senozhatsky
>
> ---
>
> drivers/staging/zram/zram_drv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 91d94b5..17386e2 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
> bio_vec *bvec, u32 index,
> {
> int ret;
>
> + if (zram->slot_free_rq)
> + handle_pending_slot_free(zram);
> +
Calling handle_pending_slot_free() without holding zram->lock?
That's racy.
Jerome
> if (rw == READ) {
> down_read(>lock);
> - handle_pending_slot_free(zram);
> ret = zram_bvec_read(zram, bvec, index, offset, bio);
> up_read(>lock);
> } else {
> down_write(>lock);
> - handle_pending_slot_free(zram);
> ret = zram_bvec_write(zram, bvec, index, offset);
> up_write(>lock);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
---
drivers/staging/zram/zram_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 91d94b5..17386e2 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
bio_vec *bvec, u32 index,
{
int ret;
+ if (zram-slot_free_rq)
+ handle_pending_slot_free(zram);
+
Calling handle_pending_slot_free() without holding zram-lock?
That's racy.
Jerome
if (rw == READ) {
down_read(zram-lock);
- handle_pending_slot_free(zram);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(zram-lock);
} else {
down_write(zram-lock);
- handle_pending_slot_free(zram);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(zram-lock);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage
On (09/06/13 16:42), Jerome Marchand wrote:
On 09/06/2013 03:47 PM, Sergey Senozhatsky wrote:
Calling handle_pending_slot_free() for every RW operation may
cause unneccessary slot_free_lock locking, because most likely
process will see NULL slot_free_rq. handle_pending_slot_free()
only when current detects that slot_free_rq is not NULL.
Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
---
drivers/staging/zram/zram_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c
b/drivers/staging/zram/zram_drv.c
index 91d94b5..17386e2 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -532,14 +532,15 @@ static int zram_bvec_rw(struct zram *zram, struct
bio_vec *bvec, u32 index,
{
int ret;
+ if (zram-slot_free_rq)
+ handle_pending_slot_free(zram);
+
Calling handle_pending_slot_free() without holding zram-lock?
That's racy.
sorry, my bad. it should take down_write() lock.
-ss
Jerome
if (rw == READ) {
down_read(zram-lock);
- handle_pending_slot_free(zram);
ret = zram_bvec_read(zram, bvec, index, offset, bio);
up_read(zram-lock);
} else {
down_write(zram-lock);
- handle_pending_slot_free(zram);
ret = zram_bvec_write(zram, bvec, index, offset);
up_write(zram-lock);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
