* [PATCH] 9pfs: Convert reclaim list to QSLIST
@ 2021-01-22 14:35 Greg Kurz
2021-01-22 17:18 ` Christian Schoenebeck
0 siblings, 1 reply; 2+ messages in thread
From: Greg Kurz @ 2021-01-22 14:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz
Use QSLIST instead of open-coding for a slightly improved readability.
No behavioral change.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
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);
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 {
--
2.26.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] 9pfs: Convert reclaim list to QSLIST
2021-01-22 14:35 [PATCH] 9pfs: Convert reclaim list to QSLIST Greg Kurz
@ 2021-01-22 17:18 ` Christian Schoenebeck
0 siblings, 0 replies; 2+ messages in thread
From: Christian Schoenebeck @ 2021-01-22 17:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-01-22 17:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-22 14:35 [PATCH] 9pfs: Convert reclaim list to QSLIST Greg Kurz
2021-01-22 17:18 ` Christian Schoenebeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).