* [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length
@ 2010-09-30 18:47 Benny Halevy
2010-10-01 14:50 ` J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: Benny Halevy @ 2010-09-30 18:47 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
The existing code adjusted it based on the worst case scenario for the returned
bitmap and the best case scenario for the supported attrs attribute.
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
fs/nfsd/nfs4xdr.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 1a468bb..f48d891 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1805,19 +1805,23 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
goto out_nfserr;
}
}
- if ((buflen -= 16) < 0)
- goto out_resource;
if (unlikely(bmval2)) {
+ if ((buflen -= 16) < 0)
+ goto out_resource;
WRITE32(3);
WRITE32(bmval0);
WRITE32(bmval1);
WRITE32(bmval2);
} else if (likely(bmval1)) {
+ if ((buflen -= 12) < 0)
+ goto out_resource;
WRITE32(2);
WRITE32(bmval0);
WRITE32(bmval1);
} else {
+ if ((buflen -= 8) < 0)
+ goto out_resource;
WRITE32(1);
WRITE32(bmval0);
}
@@ -1828,15 +1832,17 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
u32 word1 = nfsd_suppattrs1(minorversion);
u32 word2 = nfsd_suppattrs2(minorversion);
- if ((buflen -= 12) < 0)
- goto out_resource;
if (!aclsupport)
word0 &= ~FATTR4_WORD0_ACL;
if (!word2) {
+ if ((buflen -= 12) < 0)
+ goto out_resource;
WRITE32(2);
WRITE32(word0);
WRITE32(word1);
} else {
+ if ((buflen -= 16) < 0)
+ goto out_resource;
WRITE32(3);
WRITE32(word0);
WRITE32(word1);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length
2010-09-30 18:47 [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length Benny Halevy
@ 2010-10-01 14:50 ` J. Bruce Fields
2010-10-01 15:18 ` Benny Halevy
0 siblings, 1 reply; 4+ messages in thread
From: J. Bruce Fields @ 2010-10-01 14:50 UTC (permalink / raw)
To: Benny Halevy; +Cc: linux-nfs
On Thu, Sep 30, 2010 at 08:47:46PM +0200, Benny Halevy wrote:
> The existing code adjusted it based on the worst case scenario for the returned
> bitmap and the best case scenario for the supported attrs attribute.
Looks fine, thanks. Mind if I just ditch the unlikely()s? I know,
they've been there for a while, but I'm just skeptical of their value on
anything but the most unbelievably hot optimized-to-death code paths.
--b.
>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> fs/nfsd/nfs4xdr.c | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1a468bb..f48d891 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1805,19 +1805,23 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
> goto out_nfserr;
> }
> }
> - if ((buflen -= 16) < 0)
> - goto out_resource;
>
> if (unlikely(bmval2)) {
> + if ((buflen -= 16) < 0)
> + goto out_resource;
> WRITE32(3);
> WRITE32(bmval0);
> WRITE32(bmval1);
> WRITE32(bmval2);
> } else if (likely(bmval1)) {
> + if ((buflen -= 12) < 0)
> + goto out_resource;
> WRITE32(2);
> WRITE32(bmval0);
> WRITE32(bmval1);
> } else {
> + if ((buflen -= 8) < 0)
> + goto out_resource;
> WRITE32(1);
> WRITE32(bmval0);
> }
> @@ -1828,15 +1832,17 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
> u32 word1 = nfsd_suppattrs1(minorversion);
> u32 word2 = nfsd_suppattrs2(minorversion);
>
> - if ((buflen -= 12) < 0)
> - goto out_resource;
> if (!aclsupport)
> word0 &= ~FATTR4_WORD0_ACL;
> if (!word2) {
> + if ((buflen -= 12) < 0)
> + goto out_resource;
> WRITE32(2);
> WRITE32(word0);
> WRITE32(word1);
> } else {
> + if ((buflen -= 16) < 0)
> + goto out_resource;
> WRITE32(3);
> WRITE32(word0);
> WRITE32(word1);
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length
2010-10-01 14:50 ` J. Bruce Fields
@ 2010-10-01 15:18 ` Benny Halevy
2010-10-01 22:05 ` J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: Benny Halevy @ 2010-10-01 15:18 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
On 2010-10-01 16:50, J. Bruce Fields wrote:
> On Thu, Sep 30, 2010 at 08:47:46PM +0200, Benny Halevy wrote:
>> The existing code adjusted it based on the worst case scenario for the returned
>> bitmap and the best case scenario for the supported attrs attribute.
>
> Looks fine, thanks. Mind if I just ditch the unlikely()s? I know,
> they've been there for a while, but I'm just skeptical of their value on
> anything but the most unbelievably hot optimized-to-death code paths.
Well, they don't bother me so I don't the value of ditching them either :)
They're supposed to help branch prediction thus optimizing the common
code paths on the expense of the unlikely path but I too didn't measure
their affect.
Bottom line is that I don't have and strong reservations either for or
against them in this case :)
Benny
>
> --b.
>
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>> fs/nfsd/nfs4xdr.c | 14 ++++++++++----
>> 1 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 1a468bb..f48d891 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1805,19 +1805,23 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>> goto out_nfserr;
>> }
>> }
>> - if ((buflen -= 16) < 0)
>> - goto out_resource;
>>
>> if (unlikely(bmval2)) {
>> + if ((buflen -= 16) < 0)
>> + goto out_resource;
>> WRITE32(3);
>> WRITE32(bmval0);
>> WRITE32(bmval1);
>> WRITE32(bmval2);
>> } else if (likely(bmval1)) {
>> + if ((buflen -= 12) < 0)
>> + goto out_resource;
>> WRITE32(2);
>> WRITE32(bmval0);
>> WRITE32(bmval1);
>> } else {
>> + if ((buflen -= 8) < 0)
>> + goto out_resource;
>> WRITE32(1);
>> WRITE32(bmval0);
>> }
>> @@ -1828,15 +1832,17 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>> u32 word1 = nfsd_suppattrs1(minorversion);
>> u32 word2 = nfsd_suppattrs2(minorversion);
>>
>> - if ((buflen -= 12) < 0)
>> - goto out_resource;
>> if (!aclsupport)
>> word0 &= ~FATTR4_WORD0_ACL;
>> if (!word2) {
>> + if ((buflen -= 12) < 0)
>> + goto out_resource;
>> WRITE32(2);
>> WRITE32(word0);
>> WRITE32(word1);
>> } else {
>> + if ((buflen -= 16) < 0)
>> + goto out_resource;
>> WRITE32(3);
>> WRITE32(word0);
>> WRITE32(word1);
>> --
>> 1.7.2.3
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length
2010-10-01 15:18 ` Benny Halevy
@ 2010-10-01 22:05 ` J. Bruce Fields
0 siblings, 0 replies; 4+ messages in thread
From: J. Bruce Fields @ 2010-10-01 22:05 UTC (permalink / raw)
To: Benny Halevy; +Cc: linux-nfs
On Fri, Oct 01, 2010 at 05:18:03PM +0200, Benny Halevy wrote:
> On 2010-10-01 16:50, J. Bruce Fields wrote:
> > On Thu, Sep 30, 2010 at 08:47:46PM +0200, Benny Halevy wrote:
> >> The existing code adjusted it based on the worst case scenario for the returned
> >> bitmap and the best case scenario for the supported attrs attribute.
> >
> > Looks fine, thanks. Mind if I just ditch the unlikely()s? I know,
> > they've been there for a while, but I'm just skeptical of their value on
> > anything but the most unbelievably hot optimized-to-death code paths.
>
> Well, they don't bother me so I don't the value of ditching them either :)
For me it's an infinitessimal optimization versus a very minor
improvement in readability.
Admittedly my scales of judgement may not be fine enough to correctly
balance such small quantities....
Anyway, applied without annotations; thanks.
--b.
>
> They're supposed to help branch prediction thus optimizing the common
> code paths on the expense of the unlikely path but I too didn't measure
> their affect.
>
> Bottom line is that I don't have and strong reservations either for or
> against them in this case :)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-10-01 22:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-30 18:47 [PATCH 1/2] nfsd4: adjust buflen for encoded attrs bitmap based on actual bitmap length Benny Halevy
2010-10-01 14:50 ` J. Bruce Fields
2010-10-01 15:18 ` Benny Halevy
2010-10-01 22:05 ` J. Bruce Fields
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).