| Message ID | 20230821034637.34630-9-byungchul@sk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DEPT(Dependency Tracker) | expand |
On Mon, Aug 21, 2023 at 12:46:20PM +0900, Byungchul Park wrote: > @@ -1219,6 +1220,9 @@ static inline bool folio_trylock_flag(struct folio *folio, int bit_nr, > /* How many times do we accept lock stealing from under a waiter? */ > int sysctl_page_lock_unfairness = 5; > > +static struct dept_map __maybe_unused PG_locked_map = DEPT_MAP_INITIALIZER(PG_locked_map, NULL); > +static struct dept_map __maybe_unused PG_writeback_map = DEPT_MAP_INITIALIZER(PG_writeback_map, NULL); Hmm, why are these "maybe unused"? *digs*. Ah. Because sdt_might_sleep_start() becomes a no-op macro if DEPT is disabled. OK, the right way to handle this is #ifdef CONFIG_DEPT #define DEPT_MAP(name) static struct dept_map name = \ DEPT_MAP_INITIALIZER(name, NULL) #else #define DEPT_MAP(name) /* */ #endif And now DEPT takes up no space if disabled. /* */; is a somewhat unusual thing to see, but since this must work at top level, we can't use "do { } while (0)" like we usually do. Given where else this is likely to be used, i don't think it's going to be a problem ...
On Mon, Aug 21, 2023 at 05:09:44AM +0100, Matthew Wilcox wrote: > On Mon, Aug 21, 2023 at 12:46:20PM +0900, Byungchul Park wrote: > > @@ -1219,6 +1220,9 @@ static inline bool folio_trylock_flag(struct folio *folio, int bit_nr, > > /* How many times do we accept lock stealing from under a waiter? */ > > int sysctl_page_lock_unfairness = 5; > > > > +static struct dept_map __maybe_unused PG_locked_map = DEPT_MAP_INITIALIZER(PG_locked_map, NULL); > > +static struct dept_map __maybe_unused PG_writeback_map = DEPT_MAP_INITIALIZER(PG_writeback_map, NULL); > > Hmm, why are these "maybe unused"? *digs*. Ah. Because > sdt_might_sleep_start() becomes a no-op macro if DEPT is disabled. > > OK, the right way to handle this is > > #ifdef CONFIG_DEPT > #define DEPT_MAP(name) static struct dept_map name = \ > DEPT_MAP_INITIALIZER(name, NULL) > #else > #define DEPT_MAP(name) /* */ > #endif > > And now DEPT takes up no space if disabled. Currently: #if !defined(CONFIG_DEPT) struct dept_map { }; #endif So I think it doesn't take space at all. Do you think I still need to introduce e.g. DEPT_MAP()? Then I will. Thank you! Byungchul > /* */; is a somewhat unusual thing to see, but since this must work at > top level, we can't use "do { } while (0)" like we usually do. Given > where else this is likely to be used, i don't think it's going to be > a problem ...
diff --git a/mm/filemap.c b/mm/filemap.c index 83dda76d1fc3..eed64dc88e43 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -44,6 +44,7 @@ #include <linux/migrate.h> #include <linux/pipe_fs_i.h> #include <linux/splice.h> +#include <linux/dept_sdt.h> #include <asm/pgalloc.h> #include <asm/tlbflush.h> #include "internal.h" @@ -1219,6 +1220,9 @@ static inline bool folio_trylock_flag(struct folio *folio, int bit_nr, /* How many times do we accept lock stealing from under a waiter? */ int sysctl_page_lock_unfairness = 5; +static struct dept_map __maybe_unused PG_locked_map = DEPT_MAP_INITIALIZER(PG_locked_map, NULL); +static struct dept_map __maybe_unused PG_writeback_map = DEPT_MAP_INITIALIZER(PG_writeback_map, NULL); + static inline int folio_wait_bit_common(struct folio *folio, int bit_nr, int state, enum behavior behavior) { @@ -1230,6 +1234,11 @@ static inline int folio_wait_bit_common(struct folio *folio, int bit_nr, unsigned long pflags; bool in_thrashing; + if (bit_nr == PG_locked) + sdt_might_sleep_start(&PG_locked_map); + else if (bit_nr == PG_writeback) + sdt_might_sleep_start(&PG_writeback_map); + if (bit_nr == PG_locked && !folio_test_uptodate(folio) && folio_test_workingset(folio)) { delayacct_thrashing_start(&in_thrashing); @@ -1331,6 +1340,8 @@ static inline int folio_wait_bit_common(struct folio *folio, int bit_nr, */ finish_wait(q, wait); + sdt_might_sleep_end(); + if (thrashing) { delayacct_thrashing_end(&in_thrashing); psi_memstall_leave(&pflags);
Makes Dept able to track dependencies by PG_{locked,writeback} waits. Signed-off-by: Byungchul Park <byungchul@sk.com> --- mm/filemap.c | 11 +++++++++++ 1 file changed, 11 insertions(+)