linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Rename tsk_restore_flags() to current_restore_flags()
@ 2017-04-07  0:03 NeilBrown
  2017-04-07  8:04 ` Michal Hocko
  0 siblings, 1 reply; 2+ messages in thread
From: NeilBrown @ 2017-04-07  0:03 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra; +Cc: Michal Hocko, Mel Gorman, LKML

[-- Attachment #1: Type: text/plain, Size: 4379 bytes --]


It is not safe for one thread to modify the ->flags
of another thread as there is no locking that can protect
the update.
So tsk_restore_flags(), which takes a task pointer and modifies
the flags, is an invitation to do the wrong thing.

All current users pass "current" as the task, so no developers have
accepted that invitation.  It would be best to ensure it remains
that way.

So rename tsk_restore_flags() to current_restore_flags() and don't
pass in a task_struct pointer.  Always operate on current->flags.

Signed-off-by: NeilBrown <neilb@suse.com>
---

get_maintainer.pl suggested 35 recipients for this patch.
I decided to trim that a bit ....

Can it just go through the tip tree??  It should be completely
uncontroversial.

Thanks,
NeilBrown

 drivers/block/nbd.c      | 2 +-
 drivers/scsi/iscsi_tcp.c | 2 +-
 fs/nfsd/vfs.c            | 2 +-
 include/linux/sched.h    | 6 +++---
 kernel/softirq.c         | 2 +-
 net/core/dev.c           | 2 +-
 net/core/sock.c          | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7e4287bc19e5..04ec921727b7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -239,7 +239,7 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send,
 		}
 	} while (msg_data_left(&msg));
 
-	tsk_restore_flags(current, pflags, PF_MEMALLOC);
+	current_restore_flags(pflags, PF_MEMALLOC);
 
 	return result;
 }
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 4228aba1f654..bbea8eac9abb 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -387,7 +387,7 @@ static int iscsi_sw_tcp_pdu_xmit(struct iscsi_task *task)
 		rc = 0;
 	}
 
-	tsk_restore_flags(current, pflags, PF_MEMALLOC);
+	current_restore_flags(pflags, PF_MEMALLOC);
 	return rc;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 19d50f600e8d..9aaf6ca77569 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1004,7 +1004,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 	else
 		err = nfserrno(host_err);
 	if (test_bit(RQ_LOCAL, &rqstp->rq_flags))
-		tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
+		current_restore_flags(pflags, PF_LESS_THROTTLE);
 	return err;
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee84fd43..0978fb74e45a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1286,10 +1286,10 @@ TASK_PFA_TEST(LMK_WAITING, lmk_waiting)
 TASK_PFA_SET(LMK_WAITING, lmk_waiting)
 
 static inline void
-tsk_restore_flags(struct task_struct *task, unsigned long orig_flags, unsigned long flags)
+current_restore_flags(unsigned long orig_flags, unsigned long flags)
 {
-	task->flags &= ~flags;
-	task->flags |= orig_flags & flags;
+	current->flags &= ~flags;
+	current->flags |= orig_flags & flags;
 }
 
 extern int cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpumask *trial);
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 744fa611cae0..4e09821f9d9e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -309,7 +309,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 	account_irq_exit_time(current);
 	__local_bh_enable(SOFTIRQ_OFFSET);
 	WARN_ON_ONCE(in_interrupt());
-	tsk_restore_flags(current, old_flags, PF_MEMALLOC);
+	current_restore_flags(old_flags, PF_MEMALLOC);
 }
 
 asmlinkage __visible void do_softirq(void)
diff --git a/net/core/dev.c b/net/core/dev.c
index 7869ae3837ca..e8a366387a99 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4240,7 +4240,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 		 */
 		current->flags |= PF_MEMALLOC;
 		ret = __netif_receive_skb_core(skb, true);
-		tsk_restore_flags(current, pflags, PF_MEMALLOC);
+		current_restore_flags(pflags, PF_MEMALLOC);
 	} else
 		ret = __netif_receive_skb_core(skb, false);
 
diff --git a/net/core/sock.c b/net/core/sock.c
index a96d5f7a5734..20fb01b01ddb 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -325,7 +325,7 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
 
 	current->flags |= PF_MEMALLOC;
 	ret = sk->sk_backlog_rcv(sk, skb);
-	tsk_restore_flags(current, pflags, PF_MEMALLOC);
+	current_restore_flags(pflags, PF_MEMALLOC);
 
 	return ret;
 }
-- 
2.12.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] Rename tsk_restore_flags() to current_restore_flags()
  2017-04-07  0:03 [PATCH] Rename tsk_restore_flags() to current_restore_flags() NeilBrown
@ 2017-04-07  8:04 ` Michal Hocko
  0 siblings, 0 replies; 2+ messages in thread
From: Michal Hocko @ 2017-04-07  8:04 UTC (permalink / raw)
  To: NeilBrown; +Cc: Ingo Molnar, Peter Zijlstra, Mel Gorman, LKML

On Fri 07-04-17 10:03:26, NeilBrown wrote:
> 
> It is not safe for one thread to modify the ->flags
> of another thread as there is no locking that can protect
> the update.
> So tsk_restore_flags(), which takes a task pointer and modifies
> the flags, is an invitation to do the wrong thing.
> 
> All current users pass "current" as the task, so no developers have
> accepted that invitation.  It would be best to ensure it remains
> that way.
> 
> So rename tsk_restore_flags() to current_restore_flags() and don't
> pass in a task_struct pointer.  Always operate on current->flags.

yes this makes a lot of sense to me. 

It would be great if somebody cleaned up setting of ->flags to be always
set on current as well. This seems to be the case but there are places
where we do
	struct task_struct *tsk = current;
	[...]
	tsk->flags |= PF_$FOO


> Signed-off-by: NeilBrown <neilb@suse.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
> 
> get_maintainer.pl suggested 35 recipients for this patch.
> I decided to trim that a bit ....
> 
> Can it just go through the tip tree??  It should be completely
> uncontroversial.
> 
> Thanks,
> NeilBrown
> 
>  drivers/block/nbd.c      | 2 +-
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  fs/nfsd/vfs.c            | 2 +-
>  include/linux/sched.h    | 6 +++---
>  kernel/softirq.c         | 2 +-
>  net/core/dev.c           | 2 +-
>  net/core/sock.c          | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 7e4287bc19e5..04ec921727b7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -239,7 +239,7 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send,
>  		}
>  	} while (msg_data_left(&msg));
>  
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  
>  	return result;
>  }
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 4228aba1f654..bbea8eac9abb 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -387,7 +387,7 @@ static int iscsi_sw_tcp_pdu_xmit(struct iscsi_task *task)
>  		rc = 0;
>  	}
>  
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  	return rc;
>  }
>  
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 19d50f600e8d..9aaf6ca77569 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1004,7 +1004,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
>  	else
>  		err = nfserrno(host_err);
>  	if (test_bit(RQ_LOCAL, &rqstp->rq_flags))
> -		tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
> +		current_restore_flags(pflags, PF_LESS_THROTTLE);
>  	return err;
>  }
>  
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee84fd43..0978fb74e45a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1286,10 +1286,10 @@ TASK_PFA_TEST(LMK_WAITING, lmk_waiting)
>  TASK_PFA_SET(LMK_WAITING, lmk_waiting)
>  
>  static inline void
> -tsk_restore_flags(struct task_struct *task, unsigned long orig_flags, unsigned long flags)
> +current_restore_flags(unsigned long orig_flags, unsigned long flags)
>  {
> -	task->flags &= ~flags;
> -	task->flags |= orig_flags & flags;
> +	current->flags &= ~flags;
> +	current->flags |= orig_flags & flags;
>  }
>  
>  extern int cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpumask *trial);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 744fa611cae0..4e09821f9d9e 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -309,7 +309,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
>  	account_irq_exit_time(current);
>  	__local_bh_enable(SOFTIRQ_OFFSET);
>  	WARN_ON_ONCE(in_interrupt());
> -	tsk_restore_flags(current, old_flags, PF_MEMALLOC);
> +	current_restore_flags(old_flags, PF_MEMALLOC);
>  }
>  
>  asmlinkage __visible void do_softirq(void)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7869ae3837ca..e8a366387a99 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4240,7 +4240,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
>  		 */
>  		current->flags |= PF_MEMALLOC;
>  		ret = __netif_receive_skb_core(skb, true);
> -		tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +		current_restore_flags(pflags, PF_MEMALLOC);
>  	} else
>  		ret = __netif_receive_skb_core(skb, false);
>  
> diff --git a/net/core/sock.c b/net/core/sock.c
> index a96d5f7a5734..20fb01b01ddb 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -325,7 +325,7 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  
>  	current->flags |= PF_MEMALLOC;
>  	ret = sk->sk_backlog_rcv(sk, skb);
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  
>  	return ret;
>  }
> -- 
> 2.12.2
> 



-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2017-04-07  8:04 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07  0:03 [PATCH] Rename tsk_restore_flags() to current_restore_flags() NeilBrown
2017-04-07  8:04 ` Michal Hocko

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).