On 2015/09/28, 2:19 AM, "lustre-devel on behalf of Arnd Bergmann"
<lustre-devel-bounces@xxxxxxxxxxxxxxxx on behalf of arnd@xxxxxxxx> wrote:
>On Monday 28 September 2015 01:09:18 Dilger, Andreas wrote:
>> On 2015/09/27, 10:45 PM, "green@xxxxxxxxxxxxxx" <green@xxxxxxxxxxxxxx>
>> wrote:
>> >diff --git a/drivers/staging/lustre/lustre/ptlrpc/events.c
>> >b/drivers/staging/lustre/lustre/ptlrpc/events.c
>> >index 53f6b62..afd869b 100644
>> >--- a/drivers/staging/lustre/lustre/ptlrpc/events.c
>> >+++ b/drivers/staging/lustre/lustre/ptlrpc/events.c
>> >@@ -246,7 +246,7 @@ static void ptlrpc_req_add_history(struct
>> >ptlrpc_service_part *svcpt,
>> > struct ptlrpc_request *req)
>> > {
>> > __u64 sec = req->rq_arrival_time.tv_sec;
>> >- __u32 usec = req->rq_arrival_time.tv_usec >> 4; /* usec / 16 */
>> >+ __u32 usec = req->rq_arrival_time.tv_nsec / NSEC_PER_USEC / 16; /*
>>usec
>> >/ 16 */
>>
>> This could just be written like:
>>
>> __u32 usec = req->rq_arrival_time.tv_nsec >> 14 /* nsec / 16384 */;
>>
>> since the main point of this calculation is to get a number that fits
>> into a 16-bit field to provide ordering for items in a trace log. It
>> doesn't have to be exactly "nsec / 16000", and it avoids the division.
>
>Ok, that wasn't clear from the original code, so I just moved the division
>from the do_gettimeofday() here to keep the data unchanged.
>
>With your change, the
>
>new_seq = (sec << REQS_SEC_SHIFT) |
> (usec << REQS_USEC_SHIFT) |
> (svcpt->scp_cpt < 0 ? 0 : svcpt->scp_cpt);
>
>calculation will get forward jumps once a second, but I guess that
>doesn't matter if it's only used for sequencing.
>
>The part that I had not noticed here is the y2106 overflow in the
>sequence number. If we change the logic, we should probably give
>a few more bits to the seconds, as well, or use monotonic time.
>
>> >diff --git a/drivers/staging/lustre/lustre/ptlrpc/service.c
>> >b/drivers/staging/lustre/lustre/ptlrpc/service.c
>> >index 40de622..28f57d7 100644
>> >--- a/drivers/staging/lustre/lustre/ptlrpc/service.c
>> >+++ b/drivers/staging/lustre/lustre/ptlrpc/service.c
>> >@@ -1191,7 +1191,7 @@ static int ptlrpc_at_add_timed(struct
>> >ptlrpc_request *req)
>> > spin_lock(&svcpt->scp_at_lock);
>> > LASSERT(list_empty(&req->rq_timed_list));
>> >
>> >- index = (unsigned long)req->rq_deadline % array->paa_size;
>> >+ div_u64_rem(req->rq_deadline, array->paa_size, &index);
>>
>> Since this is just a round-robin index that advances once per second,
>> it doesn't matter at all whether the calculation is computed on the
>> 64-bit seconds or on the 32-bit seconds, so there isn't any need for
>> the more expensive div_u64_rem() call here at all. It is fine to
>> just truncate the seconds and then do the modulus on the 32-bit value.
>>
>> >@@ -1421,7 +1421,7 @@ static int ptlrpc_at_check_timed(struct
>> >ptlrpc_service_part *svcpt)
>> > server will take. Send early replies to everyone expiring soon. */
>> > INIT_LIST_HEAD(&work_list);
>> > deadline = -1;
>> >- index = (unsigned long)array->paa_deadline % array->paa_size;
>> >+ div_u64_rem(array->paa_deadline, array->paa_size, &index);
>>
>> Same here.
>
>I went back and forth on these. Initially I did just what you suggest
>here and added a (u32) cast on the deadline fields, but I could not
>convince myself that the backwards jump in 2038 is harmless. For all
>I can tell, array->paa_size is not normally a power-of-two number, so
>(0xffffffff % array->paa_size) and (0 % array->paa_size) are not
>neighboring indices. If you are sure that the index can be allowed to
>jump in 2038, we should do the simpler math and add a comment.
The jump should be largely harmless. This array is tracking the maximum
RPC service time over the past paa_size interval to avoid sending spurious
RPC retries if the server is already heavily loaded. If these values
are not expired in 100% accurate order the worst thing that can happen is
that some clients may resend their RPCs unnecessarily under heavy load.
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel High Performance Data Division
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
