[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] 9pfs: Convert reclaim list to QSLIST
From: |
Christian Schoenebeck |
Subject: |
Re: [PATCH] 9pfs: Convert reclaim list to QSLIST |
Date: |
Fri, 22 Jan 2021 18:18:01 +0100 |
On Freitag, 22. Januar 2021 15:35:14 CET Greg Kurz wrote:
> Use QSLIST instead of open-coding for a slightly improved readability.
>
> No behavioral change.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
In general LGTM, so:
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Some comments below.
> ---
> hw/9pfs/9p.c | 17 ++++++++---------
> hw/9pfs/9p.h | 2 +-
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 3864d014b43c..5a6e2c9d3d7f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -416,7 +416,9 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> {
> int reclaim_count = 0;
> V9fsState *s = pdu->s;
> - V9fsFidState *f, *reclaim_list = NULL;
> + V9fsFidState *f;
> + QSLIST_HEAD(, V9fsFidState) reclaim_list =
> + QSLIST_HEAD_INITIALIZER(reclaim_list);
>
> QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
> /*
> @@ -448,8 +450,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> * a clunk request won't free this fid
> */
> f->ref++;
> - f->rclm_lst = reclaim_list;
> - reclaim_list = f;
> + QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
Yeah, that's actually much more readable this way.
> f->fs_reclaim.fd = f->fs.fd;
> f->fs.fd = -1;
> reclaim_count++;
> @@ -461,8 +462,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> * a clunk request won't free this fid
> */
> f->ref++;
> - f->rclm_lst = reclaim_list;
> - reclaim_list = f;
> + QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
> f->fs_reclaim.dir.stream = f->fs.dir.stream;
> f->fs.dir.stream = NULL;
> reclaim_count++;
> @@ -476,15 +476,14 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> * Now close the fid in reclaim list. Free them if they
> * are already clunked.
> */
> - while (reclaim_list) {
> - f = reclaim_list;
> - reclaim_list = f->rclm_lst;
> + while (!QSLIST_EMPTY(&reclaim_list)) {
> + f = QSLIST_FIRST(&reclaim_list);
> + QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
> if (f->fid_type == P9_FID_FILE) {
> v9fs_co_close(pdu, &f->fs_reclaim);
> } else if (f->fid_type == P9_FID_DIR) {
> v9fs_co_closedir(pdu, &f->fs_reclaim);
> }
> - f->rclm_lst = NULL;
> /*
> * Now drop the fid reference, free it
> * if clunked.
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 85fb6930b0ca..00381591ffa2 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -281,7 +281,7 @@ struct V9fsFidState {
> int ref;
> bool clunked;
> QSIMPLEQ_ENTRY(V9fsFidState) next;
> - V9fsFidState *rclm_lst;
> + QSLIST_ENTRY(V9fsFidState) reclaim_next;
> };
>
> typedef enum AffixType_t {
The following is actually independent of this patch here, but related. I don't
know about you, but this looks weird to me:
static void __attribute__((__constructor__)) v9fs_set_fd_limit(void)
{
struct rlimit rlim;
if (getrlimit(RLIMIT_NOFILE, &rlim) < 0) {
error_report("Failed to get the resource limit");
exit(1);
}
open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
open_fd_rc = rlim.rlim_cur / 2;
}
'open_fd_rc' is supposed to be the soft limit I guess and 'open_fd_hw' the
hard limit. One thing is this combination of arbitrary divisions + MIN() +
arbitrary constant value of 400, but okay, the calculation does not appear to
be wrong at least.
The other thing is how reliable is it to assume this resource limit to be
constant for the entire process life time? I know on Linux this is usually not
an issue as you can have plenty FDs, but macOS for instance (on my TODO list)
is rather stingy when it comes to the soft limit of a process' FDs. Might
account to BSD as well.
I also have some doubts about how reliable the overall reclaim algorithm
actually is. It takes a while to actually hit this algorithm in practice.
Probably worth adding a test case 'one day'^TM.
Best regards,
Christian Schoenebeck