Re: [Y2038] [PATCH v3] net: rxrpc: Replace time_t with time64_t

Hi,

I have a question concerning put_time fields.

...

diff --git a/net/rxrpc/ar-connection.c b/net/rxrpc/ar-connection.c index 6631f4f..692b3e6 100644 --- a/net/rxrpc/ar-connection.c +++ b/net/rxrpc/ar-connection.c @@ -808,7 +808,7 @@ void rxrpc_put_connection(struct rxrpc_connection *conn)

  ASSERTCMP(atomic_read(&conn->usage), >, 0);

• conn->put_time = get_seconds();
  

• conn->put_time = ktime_get_seconds();
  if (atomic_dec_and_test(&conn->usage)) {
          _debug("zombie");
          rxrpc_queue_delayed_work(&rxrpc_connection_reap, 0);
  

@@ -852,7 +852,7 @@ static void rxrpc_connection_reaper(struct work_struct *work)

  _enter("");

• now = get_seconds();
  

• now = ktime_get_seconds();
  earliest = ULONG_MAX;
  
  write_lock_bh(&rxrpc_connection_lock);
  

diff --git a/net/rxrpc/ar-transport.c b/net/rxrpc/ar-transport.c index 1976dec..9946467 100644 --- a/net/rxrpc/ar-transport.c +++ b/net/rxrpc/ar-transport.c @@ -189,7 +189,7 @@ void rxrpc_put_transport(struct rxrpc_transport *trans)

  ASSERTCMP(atomic_read(&trans->usage), >, 0);

• trans->put_time = get_seconds();
  

• trans->put_time = ktime_get_seconds();
  

ktime_get_seconds returns time64_t

...

  if (unlikely(atomic_dec_and_test(&trans->usage))) {
          _debug("zombie");
          /* let the reaper determine the timeout to avoid a race with

@@ -226,7 +226,7 @@ static void rxrpc_transport_reaper(struct work_struct *work)

  _enter("");

• now = get_seconds();
  

• now = ktime_get_seconds();
  earliest = ULONG_MAX;
  
  /* extract all the transports that have been dead too long */
  

The put_time changes look much simpler to me than the rest, and this can easily be split out from the rest of the changes into a separate self-contained patch, making it clear that those times are never transferred across the wire.

...

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h index aef1bd2..24207db 100644 --- a/net/rxrpc/ar-internal.h +++ b/net/rxrpc/ar-internal.h @@ -208,7 +208,7 @@ struct rxrpc_transport { struct rb_root server_conns; /* server connections on this transport */ struct list_head link; /* link in master session list */ struct sk_buff_head error_queue; /* error packets awaiting processing */

• time_t                  put_time;       /* time at which to reap */
  

• time64_t                        put_time;       /* time at which to reap */
  spinlock_t              client_lock;    /* client connection allocation lock */
  rwlock_t                conn_lock;      /* lock for active/dead connections */
  atomic_t                usage;
  

@@ -256,7 +256,7 @@ struct rxrpc_connection { struct rxrpc_crypt csum_iv; /* packet checksum base */ unsigned long events; #define RXRPC_CONN_CHALLENGE 0 /* send challenge packet */

• time_t                  put_time;       /* time at which to reap */
  

• time64_t                        put_time;       /* time at which to reap */
  rwlock_t                lock;           /* access lock */
  spinlock_t              state_lock;     /* state-change lock */
  atomic_t                usage;
  

I'd use "unsigned long" as the type here, matching what you have in the local variables in rxrpc_connection_reaper, and change the comment to reflect that this is monotonic time.

If i use unsigned long instead time64_t then I cannot use ktime_get_seconds? Should I then leave get_seconds or use some other function?

We know that a 32-bit number is enough to store a monotonic time value (because we don't support systems don't get rebooted for 136 years), but we should also remove the 'time_t' getting mentioned here.

Thanks,

Ksenija