nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
to get a reference after finding it in the hash. Take the
rcu_read_lock() and call rhashtable_lookup directly.
Switch to using rhashtable_lookup_insert_key as well, and use the usual
retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
target as well.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
1 file changed, 20 insertions(+), 26 deletions(-)
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index be152e3e3a80..63955f3353ed 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
.need = may_flags & NFSD_FILE_MAY_MASK,
.net = SVC_NET(rqstp),
};
- struct nfsd_file *nf, *new;
+ struct nfsd_file *nf;
bool retry = true;
__be32 status;
+ int ret;
status = fh_verify(rqstp, fhp, S_IFREG,
may_flags|NFSD_MAY_OWNER_OVERRIDE);
@@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
key.cred = get_current_cred();
retry:
- /* Avoid allocation if the item is already in cache */
- nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
- nfsd_file_rhash_params);
+ rcu_read_lock();
+ nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
+ nfsd_file_rhash_params);
if (nf)
nf = nfsd_file_get(nf);
+ rcu_read_unlock();
if (nf)
goto wait_for_construction;
- new = nfsd_file_alloc(&key, may_flags);
- if (!new) {
+ nf = nfsd_file_alloc(&key, may_flags);
+ if (!nf) {
status = nfserr_jukebox;
goto out_status;
}
- nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
- &key, &new->nf_rhash,
- nfsd_file_rhash_params);
- if (!nf) {
- nf = new;
- goto open_file;
- }
- if (IS_ERR(nf))
- goto insert_err;
- nf = nfsd_file_get(nf);
- if (nf == NULL) {
- nf = new;
+ ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
+ &key, &nf->nf_rhash,
+ nfsd_file_rhash_params);
+ if (ret == 0)
goto open_file;
+
+ nfsd_file_slab_free(&nf->nf_rcu);
+ if (retry && ret == EEXIST) {
+ retry = false;
+ goto retry;
}
- nfsd_file_slab_free(&new->nf_rcu);
+ trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
+ status = nfserr_jukebox;
+ goto out_status;
wait_for_construction:
wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
@@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
smp_mb__after_atomic();
wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
goto out;
-
-insert_err:
- nfsd_file_slab_free(&new->nf_rcu);
- trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
- nf = NULL;
- status = nfserr_jukebox;
- goto out_status;
}
/**
--
2.37.3
> On Oct 3, 2022, at 7:34 AM, Jeff Layton <[email protected]> wrote:
>
> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> to get a reference after finding it in the hash. Take the
> rcu_read_lock() and call rhashtable_lookup directly.
>
> Switch to using rhashtable_lookup_insert_key as well, and use the usual
> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> target as well.
The insert_err goto is there to remove a very rare case from
the hot path. I'd kinda like to keep that feature of this code.
The rest of the patch looks good.
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..63955f3353ed 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> .need = may_flags & NFSD_FILE_MAY_MASK,
> .net = SVC_NET(rqstp),
> };
> - struct nfsd_file *nf, *new;
> + struct nfsd_file *nf;
> bool retry = true;
> __be32 status;
> + int ret;
>
> status = fh_verify(rqstp, fhp, S_IFREG,
> may_flags|NFSD_MAY_OWNER_OVERRIDE);
> @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> key.cred = get_current_cred();
>
> retry:
> - /* Avoid allocation if the item is already in cache */
> - nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> - nfsd_file_rhash_params);
> + rcu_read_lock();
> + nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> + nfsd_file_rhash_params);
> if (nf)
> nf = nfsd_file_get(nf);
> + rcu_read_unlock();
> if (nf)
> goto wait_for_construction;
>
> - new = nfsd_file_alloc(&key, may_flags);
> - if (!new) {
> + nf = nfsd_file_alloc(&key, may_flags);
> + if (!nf) {
> status = nfserr_jukebox;
> goto out_status;
> }
>
> - nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> - &key, &new->nf_rhash,
> - nfsd_file_rhash_params);
> - if (!nf) {
> - nf = new;
> - goto open_file;
> - }
> - if (IS_ERR(nf))
> - goto insert_err;
> - nf = nfsd_file_get(nf);
> - if (nf == NULL) {
> - nf = new;
> + ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> + &key, &nf->nf_rhash,
> + nfsd_file_rhash_params);
> + if (ret == 0)
> goto open_file;
> +
> + nfsd_file_slab_free(&nf->nf_rcu);
> + if (retry && ret == EEXIST) {
> + retry = false;
> + goto retry;
> }
> - nfsd_file_slab_free(&new->nf_rcu);
> + trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> + status = nfserr_jukebox;
> + goto out_status;
>
> wait_for_construction:
> wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> smp_mb__after_atomic();
> wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> goto out;
> -
> -insert_err:
> - nfsd_file_slab_free(&new->nf_rcu);
> - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> - nf = NULL;
> - status = nfserr_jukebox;
> - goto out_status;
> }
>
> /**
> --
> 2.37.3
>
--
Chuck Lever
On Mon, 2022-10-03 at 13:11 +0000, Chuck Lever III wrote:
>
> > On Oct 3, 2022, at 7:34 AM, Jeff Layton <[email protected]> wrote:
> >
> > nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> > to get a reference after finding it in the hash. Take the
> > rcu_read_lock() and call rhashtable_lookup directly.
> >
> > Switch to using rhashtable_lookup_insert_key as well, and use the usual
> > retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> > target as well.
>
> The insert_err goto is there to remove a very rare case from
> the hot path. I'd kinda like to keep that feature of this code.
>
IDK. I'm not sold that this goto spaghetti has any value as an
optimization. I can put it back in if you like, but I think eliminating
one of the goto targets here is a good thing.
> The rest of the patch looks good.
>
>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> > 1 file changed, 20 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index be152e3e3a80..63955f3353ed 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > .need = may_flags & NFSD_FILE_MAY_MASK,
> > .net = SVC_NET(rqstp),
> > };
> > - struct nfsd_file *nf, *new;
> > + struct nfsd_file *nf;
> > bool retry = true;
> > __be32 status;
> > + int ret;
> >
> > status = fh_verify(rqstp, fhp, S_IFREG,
> > may_flags|NFSD_MAY_OWNER_OVERRIDE);
> > @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > key.cred = get_current_cred();
> >
> > retry:
> > - /* Avoid allocation if the item is already in cache */
> > - nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> > - nfsd_file_rhash_params);
> > + rcu_read_lock();
> > + nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> > + nfsd_file_rhash_params);
> > if (nf)
> > nf = nfsd_file_get(nf);
> > + rcu_read_unlock();
> > if (nf)
> > goto wait_for_construction;
> >
> > - new = nfsd_file_alloc(&key, may_flags);
> > - if (!new) {
> > + nf = nfsd_file_alloc(&key, may_flags);
> > + if (!nf) {
> > status = nfserr_jukebox;
> > goto out_status;
> > }
> >
> > - nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> > - &key, &new->nf_rhash,
> > - nfsd_file_rhash_params);
> > - if (!nf) {
> > - nf = new;
> > - goto open_file;
> > - }
> > - if (IS_ERR(nf))
> > - goto insert_err;
> > - nf = nfsd_file_get(nf);
> > - if (nf == NULL) {
> > - nf = new;
> > + ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> > + &key, &nf->nf_rhash,
> > + nfsd_file_rhash_params);
> > + if (ret == 0)
> > goto open_file;
> > +
> > + nfsd_file_slab_free(&nf->nf_rcu);
> > + if (retry && ret == EEXIST) {
> > + retry = false;
> > + goto retry;
> > }
> > - nfsd_file_slab_free(&new->nf_rcu);
> > + trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> > + status = nfserr_jukebox;
> > + goto out_status;
> >
> > wait_for_construction:
> > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> > @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > smp_mb__after_atomic();
> > wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> > goto out;
> > -
> > -insert_err:
> > - nfsd_file_slab_free(&new->nf_rcu);
> > - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> > - nf = NULL;
> > - status = nfserr_jukebox;
> > - goto out_status;
> > }
> >
> > /**
> > --
> > 2.37.3
> >
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
On Mon, 03 Oct 2022, Jeff Layton wrote:
> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> to get a reference after finding it in the hash. Take the
> rcu_read_lock() and call rhashtable_lookup directly.
>
> Switch to using rhashtable_lookup_insert_key as well, and use the usual
> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> target as well.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..63955f3353ed 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> .need = may_flags & NFSD_FILE_MAY_MASK,
> .net = SVC_NET(rqstp),
> };
> - struct nfsd_file *nf, *new;
> + struct nfsd_file *nf;
> bool retry = true;
> __be32 status;
> + int ret;
>
> status = fh_verify(rqstp, fhp, S_IFREG,
> may_flags|NFSD_MAY_OWNER_OVERRIDE);
> @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> key.cred = get_current_cred();
>
> retry:
> - /* Avoid allocation if the item is already in cache */
> - nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> - nfsd_file_rhash_params);
> + rcu_read_lock();
> + nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> + nfsd_file_rhash_params);
> if (nf)
> nf = nfsd_file_get(nf);
> + rcu_read_unlock();
> if (nf)
> goto wait_for_construction;
>
> - new = nfsd_file_alloc(&key, may_flags);
> - if (!new) {
> + nf = nfsd_file_alloc(&key, may_flags);
> + if (!nf) {
> status = nfserr_jukebox;
> goto out_status;
> }
>
> - nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> - &key, &new->nf_rhash,
> - nfsd_file_rhash_params);
> - if (!nf) {
> - nf = new;
> - goto open_file;
> - }
> - if (IS_ERR(nf))
> - goto insert_err;
> - nf = nfsd_file_get(nf);
> - if (nf == NULL) {
> - nf = new;
> + ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> + &key, &nf->nf_rhash,
> + nfsd_file_rhash_params);
> + if (ret == 0)
> goto open_file;
> +
> + nfsd_file_slab_free(&nf->nf_rcu);
> + if (retry && ret == EEXIST) {
You need a "-" sign in there.
> + retry = false;
I can see no justification for either testing or setting "retry" here.
In the original code, limiting retries is about the "open_file" branch
hitting an error. That is totally different from a race that might
delete the file immediately after a lookup_insert failed. At most it
should be a different retry counter. If you are really concerned about
retries here, then we should stay with
rhashtable_lookup_get_insert_key().
Thanks,
NeilBrown
> + goto retry;
> }
> - nfsd_file_slab_free(&new->nf_rcu);
> + trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> + status = nfserr_jukebox;
> + goto out_status;
>
> wait_for_construction:
> wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> smp_mb__after_atomic();
> wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> goto out;
> -
> -insert_err:
> - nfsd_file_slab_free(&new->nf_rcu);
> - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> - nf = NULL;
> - status = nfserr_jukebox;
> - goto out_status;
> }
>
> /**
> --
> 2.37.3
>
>
On Tue, 04 Oct 2022, Chuck Lever III wrote:
>
> > On Oct 3, 2022, at 7:34 AM, Jeff Layton <[email protected]> wrote:
> >
> > nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> > to get a reference after finding it in the hash. Take the
> > rcu_read_lock() and call rhashtable_lookup directly.
> >
> > Switch to using rhashtable_lookup_insert_key as well, and use the usual
> > retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> > target as well.
>
> The insert_err goto is there to remove a very rare case from
> the hot path. I'd kinda like to keep that feature of this code.
????
The fast path in the new code looks quite clean - what concerns you?
Maybe a "likely()" annotation can be used to encourage the compiler to
optimise for the non-error path so the error-handling gets moved
out-of-line (assuming it isn't already), but don't think the new code
needs that goto.
NeilBrown
>
> The rest of the patch looks good.
>
>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/filecache.c | 46 ++++++++++++++++++++-------------------------
> > 1 file changed, 20 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index be152e3e3a80..63955f3353ed 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -1043,9 +1043,10 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > .need = may_flags & NFSD_FILE_MAY_MASK,
> > .net = SVC_NET(rqstp),
> > };
> > - struct nfsd_file *nf, *new;
> > + struct nfsd_file *nf;
> > bool retry = true;
> > __be32 status;
> > + int ret;
> >
> > status = fh_verify(rqstp, fhp, S_IFREG,
> > may_flags|NFSD_MAY_OWNER_OVERRIDE);
> > @@ -1055,35 +1056,35 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > key.cred = get_current_cred();
> >
> > retry:
> > - /* Avoid allocation if the item is already in cache */
> > - nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> > - nfsd_file_rhash_params);
> > + rcu_read_lock();
> > + nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> > + nfsd_file_rhash_params);
> > if (nf)
> > nf = nfsd_file_get(nf);
> > + rcu_read_unlock();
> > if (nf)
> > goto wait_for_construction;
> >
> > - new = nfsd_file_alloc(&key, may_flags);
> > - if (!new) {
> > + nf = nfsd_file_alloc(&key, may_flags);
> > + if (!nf) {
> > status = nfserr_jukebox;
> > goto out_status;
> > }
> >
> > - nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> > - &key, &new->nf_rhash,
> > - nfsd_file_rhash_params);
> > - if (!nf) {
> > - nf = new;
> > - goto open_file;
> > - }
> > - if (IS_ERR(nf))
> > - goto insert_err;
> > - nf = nfsd_file_get(nf);
> > - if (nf == NULL) {
> > - nf = new;
> > + ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> > + &key, &nf->nf_rhash,
> > + nfsd_file_rhash_params);
> > + if (ret == 0)
> > goto open_file;
> > +
> > + nfsd_file_slab_free(&nf->nf_rcu);
> > + if (retry && ret == EEXIST) {
> > + retry = false;
> > + goto retry;
> > }
> > - nfsd_file_slab_free(&new->nf_rcu);
> > + trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
> > + status = nfserr_jukebox;
> > + goto out_status;
> >
> > wait_for_construction:
> > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> > @@ -1143,13 +1144,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > smp_mb__after_atomic();
> > wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> > goto out;
> > -
> > -insert_err:
> > - nfsd_file_slab_free(&new->nf_rcu);
> > - trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> > - nf = NULL;
> > - status = nfserr_jukebox;
> > - goto out_status;
> > }
> >
> > /**
> > --
> > 2.37.3
> >
>
> --
> Chuck Lever
>
>
>
>
> On Oct 3, 2022, at 6:07 PM, NeilBrown <[email protected]> wrote:
>
> On Tue, 04 Oct 2022, Chuck Lever III wrote:
>>
>>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <[email protected]> wrote:
>>>
>>> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
>>> to get a reference after finding it in the hash. Take the
>>> rcu_read_lock() and call rhashtable_lookup directly.
>>>
>>> Switch to using rhashtable_lookup_insert_key as well, and use the usual
>>> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
>>> target as well.
>>
>> The insert_err goto is there to remove a very rare case from
>> the hot path. I'd kinda like to keep that feature of this code.
>
> ????
> The fast path in the new code looks quite clean - what concerns you?
> Maybe a "likely()" annotation can be used to encourage the compiler to
> optimise for the non-error path so the error-handling gets moved
> out-of-line (assuming it isn't already), but don't think the new code
> needs that goto.
It's an instruction cache footprint issue.
A CPU populates its instruction cache by reading instructions from
memory in bulk (some multiple of the size of the cacheline). I
would like to keep the instructions involved with very rare cases
(like this one) out-of-line so they do not clutter the CPU's
instruction cache.
Unfortunately the compiler on my system has decided to place this
snippet of code right before the out_status: label, which defeats
my intention.
--
Chuck Lever
On Wed, 05 Oct 2022, Chuck Lever III wrote:
>
> > On Oct 3, 2022, at 6:07 PM, NeilBrown <[email protected]> wrote:
> >
> > On Tue, 04 Oct 2022, Chuck Lever III wrote:
> >>
> >>> On Oct 3, 2022, at 7:34 AM, Jeff Layton <[email protected]> wrote:
> >>>
> >>> nfsd_file is RCU-freed, so we need to hold the rcu_read_lock long enough
> >>> to get a reference after finding it in the hash. Take the
> >>> rcu_read_lock() and call rhashtable_lookup directly.
> >>>
> >>> Switch to using rhashtable_lookup_insert_key as well, and use the usual
> >>> retry mechanism if we hit an -EEXIST. Eliminiate the insert_err goto
> >>> target as well.
> >>
> >> The insert_err goto is there to remove a very rare case from
> >> the hot path. I'd kinda like to keep that feature of this code.
> >
> > ????
> > The fast path in the new code looks quite clean - what concerns you?
> > Maybe a "likely()" annotation can be used to encourage the compiler to
> > optimise for the non-error path so the error-handling gets moved
> > out-of-line (assuming it isn't already), but don't think the new code
> > needs that goto.
>
> It's an instruction cache footprint issue.
>
> A CPU populates its instruction cache by reading instructions from
> memory in bulk (some multiple of the size of the cacheline). I
> would like to keep the instructions involved with very rare cases
> (like this one) out-of-line so they do not clutter the CPU's
> instruction cache.
>
> Unfortunately the compiler on my system has decided to place this
> snippet of code right before the out_status: label, which defeats
> my intention.
Don't you hate that!!!!
On the unpatched code, if I put a "likely" annotation on the assumed
common case:
if (likely(!nf)) {
nf = new;
goto open_file;
}
then the open_file code is placed immediately after the
rhashtable_lookup_get_insert_key().
This supports my suggestion that likely/unlikely annotations are better
at controlling code placement than source-code placement.
I've thought for a while that those annotations should be
optimise_for() and optimise_against() or similar. That is what is being
requested.
NeilBrown