Skip to content

Commit

Permalink
OpenZFS 9284 - arc_reclaim_thread has 2 jobs
Browse files Browse the repository at this point in the history
Following the fix for 9018 (Replace kmem_cache_reap_now() with
kmem_cache_reap_soon), the arc_reclaim_thread() no longer blocks
while reaping.  However, the code is still confusing and error-prone,
because this thread has two responsibilities.  We should instead
separate this into two threads each with their own responsibility:

 1. keep `arc_size` under `arc_c`, by calling `arc_adjust()`, which
    improves `arc_is_overflowing()`

 2. keep enough free memory in the system, by calling
    `arc_kmem_reap_now()` plus `arc_shrink()`, which improves
    `arc_available_memory()`.

Furthermore, we can use the zthr infrastructure to separate the
"should we do something" from "do it" parts of the logic, and
normalize the start up / shut down of the threads.

Authored by: Brad Lewis <[email protected]>
Reviewed by: Matt Ahrens <[email protected]>
Reviewed by: Serapheim Dimitropoulos <[email protected]>
Reviewed by: Pavel Zakharov <[email protected]>
Reviewed by: Dan Kimmel <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Reviewed by: Dan McDonald <[email protected]>
Reviewed by: Tim Kordas <[email protected]>
Reviewed by: Tim Chase <[email protected]>
Reviewed by: Brian Behlendorf <[email protected]>
Ported-by:  Brad Lewis <[email protected]>
Signed-off-by: Brad Lewis <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/9284
OpenZFS-commit: openzfs/openzfs@de753e34f9
Closes #8165
  • Loading branch information
brad-lewis authored and behlendorf committed Dec 26, 2018
1 parent 00f198d commit 3ec34e5
Show file tree
Hide file tree
Showing 7 changed files with 285 additions and 172 deletions.
2 changes: 2 additions & 0 deletions include/spl/sys/kmem.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ extern unsigned int spl_kmem_alloc_max;
#define kmem_alloc(sz, fl) spl_kmem_alloc((sz), (fl), __func__, __LINE__)
#define kmem_zalloc(sz, fl) spl_kmem_zalloc((sz), (fl), __func__, __LINE__)
#define kmem_free(ptr, sz) spl_kmem_free((ptr), (sz))
#define kmem_cache_reap_active spl_kmem_cache_reap_active

extern void *spl_kmem_alloc(size_t sz, int fl, const char *func, int line);
extern void *spl_kmem_zalloc(size_t sz, int fl, const char *func, int line);
Expand All @@ -181,5 +182,6 @@ extern void spl_kmem_free_track(const void *buf, size_t size);

extern int spl_kmem_init(void);
extern void spl_kmem_fini(void);
extern int spl_kmem_cache_reap_active(void);

#endif /* _SPL_KMEM_H */
1 change: 1 addition & 0 deletions include/sys/zfs_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,7 @@ typedef int fstrans_cookie_t;
extern fstrans_cookie_t spl_fstrans_mark(void);
extern void spl_fstrans_unmark(fstrans_cookie_t);
extern int __spl_pf_fstrans_check(void);
extern int kmem_cache_reap_active(void);

#define ____cacheline_aligned

Expand Down
4 changes: 4 additions & 0 deletions include/sys/zthr.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ struct zthr {
kmutex_t zthr_lock;
kcondvar_t zthr_cv;
boolean_t zthr_cancel;
hrtime_t zthr_wait_time;

zthr_checkfunc_t *zthr_checkfunc;
zthr_func_t *zthr_func;
Expand All @@ -38,6 +39,9 @@ struct zthr {

extern zthr_t *zthr_create(zthr_checkfunc_t checkfunc,
zthr_func_t *func, void *arg);
extern zthr_t *zthr_create_timer(zthr_checkfunc_t *checkfunc,
zthr_func_t *func, void *arg, hrtime_t nano_wait);

extern void zthr_exit(zthr_t *t, int rc);
extern void zthr_destroy(zthr_t *t);

Expand Down
6 changes: 6 additions & 0 deletions lib/libzpool/kernel.c
Original file line number Diff line number Diff line change
Expand Up @@ -1276,6 +1276,12 @@ __spl_pf_fstrans_check(void)
return (0);
}

int
kmem_cache_reap_active(void)
{
return (0);
}

void *zvol_tag = "zvol_tag";

void
Expand Down
12 changes: 12 additions & 0 deletions module/spl/spl-kmem-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -1732,6 +1732,18 @@ spl_kmem_cache_reap_now(spl_kmem_cache_t *skc, int count)
}
EXPORT_SYMBOL(spl_kmem_cache_reap_now);

/*
* This is stubbed out for code consistency with other platforms. There
* is existing logic to prevent concurrent reaping so while this is ugly
* it should do no harm.
*/
int
spl_kmem_cache_reap_active()
{
return (0);
}
EXPORT_SYMBOL(spl_kmem_cache_reap_active);

/*
* Reap all free slabs from all registered caches.
*/
Expand Down
Loading

2 comments on commit 3ec34e5

@BrainSlayer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the point of kmem_cache_reap_active if its not implemented and just returns zero. from my understanding it should track the state if a reap is already running and should prevent the event. so this might cause a race condition here since the function is not implemented

@behlendorf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kmem_cache_reap_active() was implemented this way to keep the basic flow of the code consistent with the change in OpenZFS. The way the kmem caches were implemented in the spl on Linux there already exists logic to skip a cache which is currently reaping. This was an issue we resolved in a slightly different way a while ago.

Please sign in to comment.