Clean up: re-arrange the cases in call_transmit_status() to make the
code easier to read.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/clnt.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 57f39b7..c26669c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1175,25 +1175,21 @@ call_transmit(struct rpc_task *task)
/*
* 5a. Handle cleanup after a transmission
+ * If we've been waiting on the socket's write_space()
+ * callback, or if the server is temporarily unreachable,
+ * continue holding the transport lock.
*/
static void
call_transmit_status(struct rpc_task *task)
{
+ dprint_status(task);
+
task->tk_action = call_status;
+
switch (task->tk_status) {
- case -EAGAIN:
- break;
- default:
- xprt_end_transmit(task);
- rpc_task_force_reencode(task);
+ case -EAGAIN: /* no write space */
break;
- /*
- * Special cases: if we've been waiting on the
- * socket's write_space() callback, or if the
- * socket just returned a connection error,
- * then hold onto the transport lock.
- */
- case -ECONNREFUSED:
+ case -ECONNREFUSED: /* connection problems */
case -ECONNRESET:
case -ENOTCONN:
case -EHOSTDOWN:
@@ -1206,6 +1202,10 @@ call_transmit_status(struct rpc_task *task)
break;
}
rpc_task_force_reencode(task);
+ break;
+ default: /* success, or some other error */
+ xprt_end_transmit(task);
+ rpc_task_force_reencode(task);
}
}
On Wed, 2009-10-07 at 18:02 -0400, Chuck Lever wrote:
> Clean up: re-arrange the cases in call_transmit_status() to make the
> code easier to read.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> net/sunrpc/clnt.c | 24 ++++++++++++------------
> 1 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 57f39b7..c26669c 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1175,25 +1175,21 @@ call_transmit(struct rpc_task *task)
>
> /*
> * 5a. Handle cleanup after a transmission
> + * If we've been waiting on the socket's write_space()
> + * callback, or if the server is temporarily unreachable,
> + * continue holding the transport lock.
> */
> static void
> call_transmit_status(struct rpc_task *task)
> {
> + dprint_status(task);
> +
> task->tk_action = call_status;
> +
> switch (task->tk_status) {
> - case -EAGAIN:
> - break;
> - default:
> - xprt_end_transmit(task);
> - rpc_task_force_reencode(task);
> + case -EAGAIN: /* no write space */
> break;
> - /*
> - * Special cases: if we've been waiting on the
> - * socket's write_space() callback, or if the
> - * socket just returned a connection error,
> - * then hold onto the transport lock.
> - */
> - case -ECONNREFUSED:
> + case -ECONNREFUSED: /* connection problems */
> case -ECONNRESET:
> case -ENOTCONN:
> case -EHOSTDOWN:
> @@ -1206,6 +1202,10 @@ call_transmit_status(struct rpc_task *task)
> break;
> }
> rpc_task_force_reencode(task);
> + break;
> + default: /* success, or some other error */
> + xprt_end_transmit(task);
> + rpc_task_force_reencode(task);
> }
> }
This puts the most common case (success) at the very end of the switch
statement. Most compilers will generate the most efficient code when it
is at the very beginning...
Trond
On Oct 7, 2009, at 6:22 PM, Trond Myklebust wrote:
> On Wed, 2009-10-07 at 18:02 -0400, Chuck Lever wrote:
>> Clean up: re-arrange the cases in call_transmit_status() to make the
>> code easier to read.
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> net/sunrpc/clnt.c | 24 ++++++++++++------------
>> 1 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 57f39b7..c26669c 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1175,25 +1175,21 @@ call_transmit(struct rpc_task *task)
>>
>> /*
>> * 5a. Handle cleanup after a transmission
>> + * If we've been waiting on the socket's write_space()
>> + * callback, or if the server is temporarily unreachable,
>> + * continue holding the transport lock.
>> */
>> static void
>> call_transmit_status(struct rpc_task *task)
>> {
>> + dprint_status(task);
>> +
>> task->tk_action = call_status;
>> +
>> switch (task->tk_status) {
>> - case -EAGAIN:
>> - break;
>> - default:
>> - xprt_end_transmit(task);
>> - rpc_task_force_reencode(task);
>> + case -EAGAIN: /* no write space */
>> break;
>> - /*
>> - * Special cases: if we've been waiting on the
>> - * socket's write_space() callback, or if the
>> - * socket just returned a connection error,
>> - * then hold onto the transport lock.
>> - */
>> - case -ECONNREFUSED:
>> + case -ECONNREFUSED: /* connection problems */
>> case -ECONNRESET:
>> case -ENOTCONN:
>> case -EHOSTDOWN:
>> @@ -1206,6 +1202,10 @@ call_transmit_status(struct rpc_task *task)
>> break;
>> }
>> rpc_task_force_reencode(task);
>> + break;
>> + default: /* success, or some other error */
>> + xprt_end_transmit(task);
>> + rpc_task_force_reencode(task);
>> }
>> }
>
> This puts the most common case (success) at the very end of the switch
> statement. Most compilers will generate the most efficient code when
> it
> is at the very beginning...
I dropped this one, since it's not that important.
But I wouldn't expect the compiler could optimize the default: case
the way you described. No matter what order you write these in, the
code has to check each of the other cases first before deciding to
execute the default: arm. If it's that important, maybe we should add
a "case 0:" arm?
>
> Trond
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Wed, 2009-10-07 at 19:02 -0400, Chuck Lever wrote:
> On Oct 7, 2009, at 6:22 PM, Trond Myklebust wrote:
> > On Wed, 2009-10-07 at 18:02 -0400, Chuck Lever wrote:
> >> Clean up: re-arrange the cases in call_transmit_status() to make the
> >> code easier to read.
> >>
> >> Signed-off-by: Chuck Lever <[email protected]>
> >> ---
> >>
> >> net/sunrpc/clnt.c | 24 ++++++++++++------------
> >> 1 files changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index 57f39b7..c26669c 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -1175,25 +1175,21 @@ call_transmit(struct rpc_task *task)
> >>
> >> /*
> >> * 5a. Handle cleanup after a transmission
> >> + * If we've been waiting on the socket's write_space()
> >> + * callback, or if the server is temporarily unreachable,
> >> + * continue holding the transport lock.
> >> */
> >> static void
> >> call_transmit_status(struct rpc_task *task)
> >> {
> >> + dprint_status(task);
> >> +
> >> task->tk_action = call_status;
> >> +
> >> switch (task->tk_status) {
> >> - case -EAGAIN:
> >> - break;
> >> - default:
> >> - xprt_end_transmit(task);
> >> - rpc_task_force_reencode(task);
> >> + case -EAGAIN: /* no write space */
> >> break;
> >> - /*
> >> - * Special cases: if we've been waiting on the
> >> - * socket's write_space() callback, or if the
> >> - * socket just returned a connection error,
> >> - * then hold onto the transport lock.
> >> - */
> >> - case -ECONNREFUSED:
> >> + case -ECONNREFUSED: /* connection problems */
> >> case -ECONNRESET:
> >> case -ENOTCONN:
> >> case -EHOSTDOWN:
> >> @@ -1206,6 +1202,10 @@ call_transmit_status(struct rpc_task *task)
> >> break;
> >> }
> >> rpc_task_force_reencode(task);
> >> + break;
> >> + default: /* success, or some other error */
> >> + xprt_end_transmit(task);
> >> + rpc_task_force_reencode(task);
> >> }
> >> }
> >
> > This puts the most common case (success) at the very end of the switch
> > statement. Most compilers will generate the most efficient code when
> > it
> > is at the very beginning...
>
> I dropped this one, since it's not that important.
>
> But I wouldn't expect the compiler could optimize the default: case
> the way you described. No matter what order you write these in, the
> code has to check each of the other cases first before deciding to
> execute the default: arm. If it's that important, maybe we should add
> a "case 0:" arm?
Yes. That's probably a good idea...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Oct 7, 2009, at 7:43 PM, Trond Myklebust wrote:
> On Wed, 2009-10-07 at 19:02 -0400, Chuck Lever wrote:
>> On Oct 7, 2009, at 6:22 PM, Trond Myklebust wrote:
>>> On Wed, 2009-10-07 at 18:02 -0400, Chuck Lever wrote:
>>>> Clean up: re-arrange the cases in call_transmit_status() to make
>>>> the
>>>> code easier to read.
>>>>
>>>> Signed-off-by: Chuck Lever <[email protected]>
>>>> ---
>>>>
>>>> net/sunrpc/clnt.c | 24 ++++++++++++------------
>>>> 1 files changed, 12 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>> index 57f39b7..c26669c 100644
>>>> --- a/net/sunrpc/clnt.c
>>>> +++ b/net/sunrpc/clnt.c
>>>> @@ -1175,25 +1175,21 @@ call_transmit(struct rpc_task *task)
>>>>
>>>> /*
>>>> * 5a. Handle cleanup after a transmission
>>>> + * If we've been waiting on the socket's write_space()
>>>> + * callback, or if the server is temporarily unreachable,
>>>> + * continue holding the transport lock.
>>>> */
>>>> static void
>>>> call_transmit_status(struct rpc_task *task)
>>>> {
>>>> + dprint_status(task);
>>>> +
>>>> task->tk_action = call_status;
>>>> +
>>>> switch (task->tk_status) {
>>>> - case -EAGAIN:
>>>> - break;
>>>> - default:
>>>> - xprt_end_transmit(task);
>>>> - rpc_task_force_reencode(task);
>>>> + case -EAGAIN: /* no write space */
>>>> break;
>>>> - /*
>>>> - * Special cases: if we've been waiting on the
>>>> - * socket's write_space() callback, or if the
>>>> - * socket just returned a connection error,
>>>> - * then hold onto the transport lock.
>>>> - */
>>>> - case -ECONNREFUSED:
>>>> + case -ECONNREFUSED: /* connection problems */
>>>> case -ECONNRESET:
>>>> case -ENOTCONN:
>>>> case -EHOSTDOWN:
>>>> @@ -1206,6 +1202,10 @@ call_transmit_status(struct rpc_task *task)
>>>> break;
>>>> }
>>>> rpc_task_force_reencode(task);
>>>> + break;
>>>> + default: /* success, or some other error */
>>>> + xprt_end_transmit(task);
>>>> + rpc_task_force_reencode(task);
>>>> }
>>>> }
>>>
>>> This puts the most common case (success) at the very end of the
>>> switch
>>> statement. Most compilers will generate the most efficient code when
>>> it
>>> is at the very beginning...
>>
>> I dropped this one, since it's not that important.
>>
>> But I wouldn't expect the compiler could optimize the default: case
>> the way you described. No matter what order you write these in, the
>> code has to check each of the other cases first before deciding to
>> execute the default: arm. If it's that important, maybe we should
>> add
>> a "case 0:" arm?
>
> Yes. That's probably a good idea...
Well, after looking at the actual evidence (ie objdump output)...
Unfortunately, adding "case 0:" doesn't seem to change the object
code. Nothing I tried seemed to prevent the compiler from placing the
zero case at the end of the function. I had to add
if (task->tk_status == 0) {
xprt_end_transmit(task);
rpc_task_force_reencode(task);
return;
}
switch (task->tk_status) {
...
before the compiler would put the check for zero status first.
Hopefully, open coding it would make the arrangement permanent,
regardless of any compiler optimizations.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com