[PATCH 0/6] s390/ctcm: Remove gfp_type() usage.

[PATCH 0/6] s390/ctcm: Remove gfp_type() usage.

[Sebastian Andrzej Siewior]

Folks,

in the discussion about preempt count consistency across kernel
configurations:
  https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

Linus clearly requested that code in drivers and libraries which changes
behaviour based on execution context should either be split up so that
e.g. task context invocations and BH invocations have different interfaces
or if that's not possible the context information has to be provided by the
caller which knows in which context it is executing.

This series is removing the gfp_type() usage which uses in_interrupt().
Overall it appears to be a nice cleanup according to the diffstat.
There are still two in_irq() users left in ctcm which I'm currently
struggling with.

Sebastian

[PATCH 1/6] s390/ctcm: Put struct th_header and th_sweep on stack.

[Sebastian Andrzej Siewior]

The size of struct th_header is 8 byte and the size of struct th_sweep
is 16 byte. The memory for is allocated, initialized, used and
deallocated a few lines later.

It is more efficient to avoid the allocation/free dance and keeping the
variable on stack. Especially since the compiler is smart enough to not
allocate the memory on stack but assign the values directly.

Declare struct th_sweep/th_header on stack and initialize it to zero.
Use the local variable instead of the pointer.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_fsms.c | 17 +++++-----------
 drivers/s390/net/ctcm_main.c | 39 ++++++++----------------------------
 drivers/s390/net/ctcm_mpc.c  | 21 ++++---------------
 3 files changed, 17 insertions(+), 60 deletions(-)

diff --git a/drivers/s390/net/ctcm_fsms.c b/drivers/s390/net/ctcm_fsms.c
index 661d2a49bce96..d6592d576d1dd 100644
--- a/drivers/s390/net/ctcm_fsms.c
+++ b/drivers/s390/net/ctcm_fsms.c
@@ -1220,7 +1220,7 @@ static void ctcmpc_chx_txdone(fsm_instance *fi, int event, void *arg)
 	unsigned long	duration;
 	struct sk_buff	*peekskb;
 	int		rc;
-	struct th_header *header;
+	struct th_header header = {0, };
 	struct pdu	*p_header;
 	unsigned long done_stamp = jiffies;
 
@@ -1303,24 +1303,17 @@ static void ctcmpc_chx_txdone(fsm_instance *fi, int event, void *arg)
 	/* p_header points to the last one we handled */
 	if (p_header)
 		p_header->pdu_flag |= PDU_LAST;	/*Say it's the last one*/
-	header = kzalloc(TH_HEADER_LENGTH, gfp_type());
-	if (!header) {
-		spin_unlock(&ch->collect_lock);
-		fsm_event(priv->mpcg->fsm, MPCG_EVENT_INOP, dev);
-				goto done;
-	}
-	header->th_ch_flag = TH_HAS_PDU;  /* Normal data */
+
+	header.th_ch_flag = TH_HAS_PDU;  /* Normal data */
 	ch->th_seq_num++;
-	header->th_seq_num = ch->th_seq_num;
+	header.th_seq_num = ch->th_seq_num;
 
 	CTCM_PR_DBGDATA("%s: ToVTAM_th_seq= %08x\n" ,
 					__func__, ch->th_seq_num);
 
-	memcpy(skb_push(ch->trans_skb, TH_HEADER_LENGTH), header,
+	memcpy(skb_push(ch->trans_skb, TH_HEADER_LENGTH), &header,
 		TH_HEADER_LENGTH);	/* put the TH on the packet */
 
-	kfree(header);
-
 	CTCM_PR_DBGDATA("%s: trans_skb len:%04x \n",
 		       __func__, ch->trans_skb->len);
 	CTCM_PR_DBGDATA("%s: up-to-50 bytes of trans_skb "
diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index d06809eac16d3..6c7d6bbd27406 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -599,7 +599,7 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
 	struct net_device *dev = rch->netdev;
 	struct ctcm_priv *priv;
 	struct mpc_group *grp;
-	struct th_sweep *header;
+	struct th_sweep header = { 0, };
 	struct sk_buff *sweep_skb;
 	struct channel *ch;
 	/* int rc = 0; */
@@ -623,24 +623,10 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
 				goto nomem;
 	}
 
-	header = kmalloc(TH_SWEEP_LENGTH, gfp_type());
+	header.th.th_ch_flag	= TH_SWEEP_REQ;  /* 0x0f */
+	header.sw.th_last_seq	= ch->th_seq_num;
 
-	if (!header) {
-		dev_kfree_skb_any(sweep_skb);
-		/* rc = -ENOMEM; */
-				goto nomem;
-	}
-
-	header->th.th_seg	= 0x00 ;
-	header->th.th_ch_flag	= TH_SWEEP_REQ;  /* 0x0f */
-	header->th.th_blk_flag	= 0x00;
-	header->th.th_is_xid	= 0x00;
-	header->th.th_seq_num	= 0x00;
-	header->sw.th_last_seq	= ch->th_seq_num;
-
-	skb_put_data(sweep_skb, header, TH_SWEEP_LENGTH);
-
-	kfree(header);
+	skb_put_data(sweep_skb, &header, TH_SWEEP_LENGTH);
 
 	netif_trans_update(dev);
 	skb_queue_tail(&ch->sweep_queue, sweep_skb);
@@ -666,7 +652,7 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 	struct net_device *dev = ch->netdev;
 	struct ctcm_priv *priv = dev->ml_priv;
 	struct mpc_group *grp = priv->mpcg;
-	struct th_header *header;
+	struct th_header header = { 0, };
 	struct sk_buff *nskb;
 	int rc = 0;
 	int ccw_idx;
@@ -768,24 +754,15 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 
 	ch->prof.txlen += skb->len - PDU_HEADER_LENGTH;
 
-	header = kmalloc(TH_HEADER_LENGTH, gfp_type());
-	if (!header)
-		goto nomem_exit;
-
-	header->th_seg = 0x00;
-	header->th_ch_flag = TH_HAS_PDU;  /* Normal data */
-	header->th_blk_flag = 0x00;
-	header->th_is_xid = 0x00;          /* Just data here */
+	header.th_ch_flag = TH_HAS_PDU;  /* Normal data */
 	ch->th_seq_num++;
-	header->th_seq_num = ch->th_seq_num;
+	header.th_seq_num = ch->th_seq_num;
 
 	CTCM_PR_DBGDATA("%s(%s) ToVTAM_th_seq= %08x\n" ,
 		       __func__, dev->name, ch->th_seq_num);
 
 	/* put the TH on the packet */
-	memcpy(skb_push(skb, TH_HEADER_LENGTH), header, TH_HEADER_LENGTH);
-
-	kfree(header);
+	memcpy(skb_push(skb, TH_HEADER_LENGTH), &header, TH_HEADER_LENGTH);
 
 	CTCM_PR_DBGDATA("%s(%s): skb len: %04x\n - pdu header and data for "
 			"up to 32 bytes sent to vtam:\n",
diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 85a1a4533cbeb..04a51cc89e74c 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -641,7 +641,7 @@ static void ctcmpc_send_sweep_resp(struct channel *rch)
 	struct net_device *dev = rch->netdev;
 	struct ctcm_priv *priv = dev->ml_priv;
 	struct mpc_group *grp = priv->mpcg;
-	struct th_sweep *header;
+	struct th_sweep header = { 0, };
 	struct sk_buff *sweep_skb;
 	struct channel *ch  = priv->channel[CTCM_WRITE];
 
@@ -655,23 +655,10 @@ static void ctcmpc_send_sweep_resp(struct channel *rch)
 		goto done;
 	}
 
-	header = kmalloc(sizeof(struct th_sweep), gfp_type());
+	header.th.th_ch_flag	= TH_SWEEP_RESP;
+	header.sw.th_last_seq	= ch->th_seq_num;
 
-	if (!header) {
-		dev_kfree_skb_any(sweep_skb);
-		goto done;
-	}
-
-	header->th.th_seg	= 0x00 ;
-	header->th.th_ch_flag	= TH_SWEEP_RESP;
-	header->th.th_blk_flag	= 0x00;
-	header->th.th_is_xid	= 0x00;
-	header->th.th_seq_num	= 0x00;
-	header->sw.th_last_seq	= ch->th_seq_num;
-
-	skb_put_data(sweep_skb, header, TH_SWEEP_LENGTH);
-
-	kfree(header);
+	skb_put_data(sweep_skb, &header, TH_SWEEP_LENGTH);
 
 	netif_trans_update(dev);
 	skb_queue_tail(&ch->sweep_queue, sweep_skb);
-- 
2.29.2

[PATCH 2/6] s390/ctcm: Put struct qllc on stack.

[Sebastian Andrzej Siewior]

The size of struct qllc is 2 byte. The memory for is allocated, initialized,
used and deallocated a few lines later.

It is more efficient to avoid the allocation/free dance and keeping the
variable on stack. Especially since the compiler is smart enough to not
allocate the memory on stack but assign the values directly.

Rename `qllcptr' to `qllc' and use it on stack.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_mpc.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 04a51cc89e74c..4ff51af44d338 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -2049,11 +2049,10 @@ static void mpc_action_rcvd_xid7(fsm_instance *fsm, int event, void *arg)
  */
 static int mpc_send_qllc_discontact(struct net_device *dev)
 {
-	__u32	new_len	= 0;
 	struct sk_buff   *skb;
-	struct qllc      *qllcptr;
 	struct ctcm_priv *priv = dev->ml_priv;
 	struct mpc_group *grp = priv->mpcg;
+	struct qllc	qllc;
 
 	CTCM_PR_DEBUG("%s: GROUP STATE: %s\n",
 		__func__, mpcg_state_names[grp->saved_state]);
@@ -2080,31 +2079,20 @@ static int mpc_send_qllc_discontact(struct net_device *dev)
 	case MPCG_STATE_FLOWC:
 	case MPCG_STATE_READY:
 		grp->send_qllc_disc = 2;
-		new_len = sizeof(struct qllc);
-		qllcptr = kzalloc(new_len, gfp_type() | GFP_DMA);
-		if (qllcptr == NULL) {
-			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
-				"%s(%s): qllcptr allocation error",
-						CTCM_FUNTAIL, dev->name);
-			return -ENOMEM;
-		}
-
-		qllcptr->qllc_address = 0xcc;
-		qllcptr->qllc_commands = 0x03;
-
-		skb = __dev_alloc_skb(new_len, GFP_ATOMIC);
 
+		skb = __dev_alloc_skb(sizeof(struct qllc), GFP_ATOMIC);
 		if (skb == NULL) {
 			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
 				"%s(%s): skb allocation error",
 						CTCM_FUNTAIL, dev->name);
 			priv->stats.rx_dropped++;
-			kfree(qllcptr);
 			return -ENOMEM;
 		}
 
-		skb_put_data(skb, qllcptr, new_len);
-		kfree(qllcptr);
+		qllc.qllc_address = 0xcc;
+		qllc.qllc_commands = 0x03;
+
+		skb_put_data(skb, &qllc, sizeof(struct qllc));
 
 		if (skb_headroom(skb) < 4) {
 			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
-- 
2.29.2

[PATCH 3/6] s390/ctcm: Put struct pdu on stack.

[Sebastian Andrzej Siewior]

The size of struct pdu is 8 byte. The memory is allocated, initialized,
used and deallocated a few lines later.

It is more efficient to avoid the allocation/free dance and keeping the
variable on stack. Especially since the compiler is smart enough to not
allocate the memory on stack but assign the values directly.

Add a variable pdu_hdr on stack and use it instead of p_header. p_header
is used later as a pointer without an allocation.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_main.c | 41 ++++++++++++------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 6c7d6bbd27406..b56f51806b3d0 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -649,6 +649,7 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
 static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 {
 	struct pdu *p_header;
+	struct pdu pdu_hdr;
 	struct net_device *dev = ch->netdev;
 	struct ctcm_priv *priv = dev->ml_priv;
 	struct mpc_group *grp = priv->mpcg;
@@ -666,23 +667,16 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 	if ((fsm_getstate(ch->fsm) != CTC_STATE_TXIDLE) || grp->in_sweep) {
 		spin_lock_irqsave(&ch->collect_lock, saveflags);
 		refcount_inc(&skb->users);
-		p_header = kmalloc(PDU_HEADER_LENGTH, gfp_type());
 
-		if (!p_header) {
-			spin_unlock_irqrestore(&ch->collect_lock, saveflags);
-				goto nomem_exit;
-		}
-
-		p_header->pdu_offset = skb->len;
-		p_header->pdu_proto = 0x01;
-		p_header->pdu_flag = 0x00;
+		pdu_hdr.pdu_offset = skb->len;
+		pdu_hdr.pdu_proto = 0x01;
 		if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
-			p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
+			pdu_hdr.pdu_flag = PDU_FIRST | PDU_CNTL;
 		} else {
-			p_header->pdu_flag |= PDU_FIRST;
+			pdu_hdr.pdu_flag = PDU_FIRST;
 		}
-		p_header->pdu_seq = 0;
-		memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header,
+		pdu_hdr.pdu_seq = 0;
+		memcpy(skb_push(skb, PDU_HEADER_LENGTH), &pdu_hdr,
 		       PDU_HEADER_LENGTH);
 
 		CTCM_PR_DEBUG("%s(%s): Put on collect_q - skb len: %04x \n"
@@ -692,7 +686,6 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 
 		skb_queue_tail(&ch->collect_queue, skb);
 		ch->collect_len += skb->len;
-		kfree(p_header);
 
 		spin_unlock_irqrestore(&ch->collect_lock, saveflags);
 			goto done;
@@ -722,23 +715,15 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 		}
 	}
 
-	p_header = kmalloc(PDU_HEADER_LENGTH, gfp_type());
-
-	if (!p_header)
-		goto nomem_exit;
-
-	p_header->pdu_offset = skb->len;
-	p_header->pdu_proto = 0x01;
-	p_header->pdu_flag = 0x00;
-	p_header->pdu_seq = 0;
+	pdu_hdr.pdu_offset = skb->len;
+	pdu_hdr.pdu_proto = 0x01;
+	pdu_hdr.pdu_seq = 0;
 	if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
-		p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
+		pdu_hdr.pdu_flag = PDU_FIRST | PDU_CNTL;
 	} else {
-		p_header->pdu_flag |= PDU_FIRST;
+		pdu_hdr.pdu_flag = PDU_FIRST;
 	}
-	memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header, PDU_HEADER_LENGTH);
-
-	kfree(p_header);
+	memcpy(skb_push(skb, PDU_HEADER_LENGTH), &pdu_hdr, PDU_HEADER_LENGTH);
 
 	if (ch->collect_len > 0) {
 		spin_lock_irqsave(&ch->collect_lock, saveflags);
-- 
2.29.2

[PATCH 4/6] s390/ctcm: Use explicit allocation mask in ctcmpc_unpack_skb().

[Sebastian Andrzej Siewior]

gfp_type() uses in_interrupt() to figure out the correct GFP mask.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

The call chain of ctcmpc_unpack_skb():
ctcmpc_bh()
 -> ctcmpc_unpack_skb()

ctcmpc_bh() is a tasklet handler so GFP_ATOMIC is needed.

Use GFP_ATOMIC as allocation type in ctcmpc_unpack_skb().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_mpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 4ff51af44d338..38c535f016363 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -1164,7 +1164,7 @@ static void ctcmpc_unpack_skb(struct channel *ch, struct sk_buff *pskb)
 			skb_pull(pskb, new_len); /* point to next PDU */
 		}
 	} else {
-		mpcginfo = kmalloc(sizeof(struct mpcg_info), gfp_type());
+		mpcginfo = kmalloc(sizeof(struct mpcg_info), GFP_ATOMIC);
 		if (mpcginfo == NULL)
 					goto done;
 
-- 
2.29.2

[PATCH 5/6] s390/ctcm: Use GFP_KERNEL in add_channel().

[Sebastian Andrzej Siewior]

gfp_type() uses in_interrupt() to figure out the correct GFP mask.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

The memory allocation of `ch' a few lines above is using GFP_KERNEL,
also an allocation a few lines later is using GFP_KERNEL.

Use GFP_KERNEL for the memory allocation.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index b56f51806b3d0..ca218489e0be4 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1323,7 +1323,7 @@ static int add_channel(struct ccw_device *cdev, enum ctcm_channel_types type,
 
 	ch->protocol = priv->protocol;
 	if (IS_MPC(priv)) {
-		ch->discontact_th = kzalloc(TH_HEADER_LENGTH, gfp_type());
+		ch->discontact_th = kzalloc(TH_HEADER_LENGTH, GFP_KERNEL);
 		if (ch->discontact_th == NULL)
 					goto nomem_return;
 
-- 
2.29.2

[PATCH 6/6] s390/ctcm: Use GFP_ATOMIC in ctcmpc_tx().

[Sebastian Andrzej Siewior]

gfp_type() uses in_interrupt() to figure out the correct GFP mask.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by the
caller, which usually knows the context.

ctcmpc_tx() is used as net_device_ops::ndo_start_xmit. This callback is
invoked with disabled bottom halves.

Use GFP_ATOMIC for memory allocation in ctcmpc_tx().
Remove gfp_type() since the last user is gone.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/s390/net/ctcm_main.c | 2 +-
 drivers/s390/net/ctcm_main.h | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index ca218489e0be4..ee5c3981c0061 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -905,7 +905,7 @@ static int ctcmpc_tx(struct sk_buff *skb, struct net_device *dev)
 		CTCM_D3_DUMP((char *)skb->data, min_t(int, 32, skb->len));
 
 		len =  skb->len + TH_HEADER_LENGTH + PDU_HEADER_LENGTH;
-		newskb = __dev_alloc_skb(len, gfp_type() | GFP_DMA);
+		newskb = __dev_alloc_skb(len, GFP_ATOMIC | GFP_DMA);
 
 		if (!newskb) {
 			CTCM_DBF_TEXT_(MPC_TRACE, CTC_DBF_ERROR,
diff --git a/drivers/s390/net/ctcm_main.h b/drivers/s390/net/ctcm_main.h
index 16bdf23ee02b1..90bd7b3f80c31 100644
--- a/drivers/s390/net/ctcm_main.h
+++ b/drivers/s390/net/ctcm_main.h
@@ -298,11 +298,6 @@ struct mpc_group *ctcmpc_init_mpc_group(struct ctcm_priv *priv);
 /* test if struct ctcm_priv of struct net_device has MPC protocol setting */
 #define IS_MPCDEV(dev) IS_MPC((struct ctcm_priv *)dev->ml_priv)
 
-static inline gfp_t gfp_type(void)
-{
-	return in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
-}
-
 /*
  * Definition of our link level header.
  */
-- 
2.29.2

Re: [PATCH 4/6] s390/ctcm: Use explicit allocation mask in ctcmpc_unpack_skb().

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> gfp_type() uses in_interrupt() to figure out the correct GFP mask.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be separated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> The call chain of ctcmpc_unpack_skb():
> ctcmpc_bh()
>  -> ctcmpc_unpack_skb()
> 
> ctcmpc_bh() is a tasklet handler so GFP_ATOMIC is needed.
> 
> Use GFP_ATOMIC as allocation type in ctcmpc_unpack_skb().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>

Re: [PATCH 5/6] s390/ctcm: Use GFP_KERNEL in add_channel().

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> gfp_type() uses in_interrupt() to figure out the correct GFP mask.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be separated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> The memory allocation of `ch' a few lines above is using GFP_KERNEL,
> also an allocation a few lines later is using GFP_KERNEL.
> 
> Use GFP_KERNEL for the memory allocation.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>

Re: [PATCH 6/6] s390/ctcm: Use GFP_ATOMIC in ctcmpc_tx().

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> gfp_type() uses in_interrupt() to figure out the correct GFP mask.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be separated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> ctcmpc_tx() is used as net_device_ops::ndo_start_xmit. This callback is
> invoked with disabled bottom halves.
> 
> Use GFP_ATOMIC for memory allocation in ctcmpc_tx().
> Remove gfp_type() since the last user is gone.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>

Re: [PATCH 1/6] s390/ctcm: Put struct th_header and th_sweep on stack.

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> The size of struct th_header is 8 byte and the size of struct th_sweep
> is 16 byte. The memory for is allocated, initialized, used and
> deallocated a few lines later.
> 
> It is more efficient to avoid the allocation/free dance and keeping the
> variable on stack. Especially since the compiler is smart enough to not
> allocate the memory on stack but assign the values directly.
> 
> Declare struct th_sweep/th_header on stack and initialize it to zero.
> Use the local variable instead of the pointer.
> 

Frankly, I'd much rather see us use the pointers that are returned from
skb_push() and skb_put(). No need for the on-stack & memcpy indirection.

Re: [PATCH 3/6] s390/ctcm: Put struct pdu on stack.

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> The size of struct pdu is 8 byte. The memory is allocated, initialized,
> used and deallocated a few lines later.
> 
> It is more efficient to avoid the allocation/free dance and keeping the
> variable on stack. Especially since the compiler is smart enough to not
> allocate the memory on stack but assign the values directly.
> 
> Add a variable pdu_hdr on stack and use it instead of p_header. p_header
> is used later as a pointer without an allocation.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Same comment, can we do a 

p_header = (struct pdu *) skb_push(...);
p_header->foo = bar;

instead? Adjusting skb->len for the length that we pushed of course.

> ---
>  drivers/s390/net/ctcm_main.c | 41 ++++++++++++------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index 6c7d6bbd27406..b56f51806b3d0 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -649,6 +649,7 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
>  static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
>  {
>  	struct pdu *p_header;
> +	struct pdu pdu_hdr;
>  	struct net_device *dev = ch->netdev;
>  	struct ctcm_priv *priv = dev->ml_priv;
>  	struct mpc_group *grp = priv->mpcg;
> @@ -666,23 +667,16 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
>  	if ((fsm_getstate(ch->fsm) != CTC_STATE_TXIDLE) || grp->in_sweep) {
>  		spin_lock_irqsave(&ch->collect_lock, saveflags);
>  		refcount_inc(&skb->users);
> -		p_header = kmalloc(PDU_HEADER_LENGTH, gfp_type());
>  
> -		if (!p_header) {
> -			spin_unlock_irqrestore(&ch->collect_lock, saveflags);
> -				goto nomem_exit;
> -		}
> -
> -		p_header->pdu_offset = skb->len;
> -		p_header->pdu_proto = 0x01;
> -		p_header->pdu_flag = 0x00;
> +		pdu_hdr.pdu_offset = skb->len;
> +		pdu_hdr.pdu_proto = 0x01;
>  		if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
> -			p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
> +			pdu_hdr.pdu_flag = PDU_FIRST | PDU_CNTL;
>  		} else {
> -			p_header->pdu_flag |= PDU_FIRST;
> +			pdu_hdr.pdu_flag = PDU_FIRST;
>  		}
> -		p_header->pdu_seq = 0;
> -		memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header,
> +		pdu_hdr.pdu_seq = 0;
> +		memcpy(skb_push(skb, PDU_HEADER_LENGTH), &pdu_hdr,
>  		       PDU_HEADER_LENGTH);
>  
>  		CTCM_PR_DEBUG("%s(%s): Put on collect_q - skb len: %04x \n"
> @@ -692,7 +686,6 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
>  
>  		skb_queue_tail(&ch->collect_queue, skb);
>  		ch->collect_len += skb->len;
> -		kfree(p_header);
>  
>  		spin_unlock_irqrestore(&ch->collect_lock, saveflags);
>  			goto done;
> @@ -722,23 +715,15 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
>  		}
>  	}
>  
> -	p_header = kmalloc(PDU_HEADER_LENGTH, gfp_type());
> -
> -	if (!p_header)
> -		goto nomem_exit;
> -
> -	p_header->pdu_offset = skb->len;
> -	p_header->pdu_proto = 0x01;
> -	p_header->pdu_flag = 0x00;
> -	p_header->pdu_seq = 0;
> +	pdu_hdr.pdu_offset = skb->len;
> +	pdu_hdr.pdu_proto = 0x01;
> +	pdu_hdr.pdu_seq = 0;
>  	if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
> -		p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
> +		pdu_hdr.pdu_flag = PDU_FIRST | PDU_CNTL;
>  	} else {
> -		p_header->pdu_flag |= PDU_FIRST;
> +		pdu_hdr.pdu_flag = PDU_FIRST;
>  	}
> -	memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header, PDU_HEADER_LENGTH);
> -
> -	kfree(p_header);
> +	memcpy(skb_push(skb, PDU_HEADER_LENGTH), &pdu_hdr, PDU_HEADER_LENGTH);
>  
>  	if (ch->collect_len > 0) {
>  		spin_lock_irqsave(&ch->collect_lock, saveflags);
> 

Re: [PATCH 2/6] s390/ctcm: Put struct qllc on stack.

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> The size of struct qllc is 2 byte. The memory for is allocated, initialized,
> used and deallocated a few lines later.
> 
> It is more efficient to avoid the allocation/free dance and keeping the
> variable on stack. Especially since the compiler is smart enough to not
> allocate the memory on stack but assign the values directly.
> 
> Rename `qllcptr' to `qllc' and use it on stack.
> 

Can we please shrink this down to

qllcptr = (struct qllc *) skb_put(...);
qllcptr->foo = bar;
...


> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/s390/net/ctcm_mpc.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
> index 04a51cc89e74c..4ff51af44d338 100644
> --- a/drivers/s390/net/ctcm_mpc.c
> +++ b/drivers/s390/net/ctcm_mpc.c
> @@ -2049,11 +2049,10 @@ static void mpc_action_rcvd_xid7(fsm_instance *fsm, int event, void *arg)
>   */
>  static int mpc_send_qllc_discontact(struct net_device *dev)
>  {
> -	__u32	new_len	= 0;
>  	struct sk_buff   *skb;
> -	struct qllc      *qllcptr;
>  	struct ctcm_priv *priv = dev->ml_priv;
>  	struct mpc_group *grp = priv->mpcg;
> +	struct qllc	qllc;
>  
>  	CTCM_PR_DEBUG("%s: GROUP STATE: %s\n",
>  		__func__, mpcg_state_names[grp->saved_state]);
> @@ -2080,31 +2079,20 @@ static int mpc_send_qllc_discontact(struct net_device *dev)
>  	case MPCG_STATE_FLOWC:
>  	case MPCG_STATE_READY:
>  		grp->send_qllc_disc = 2;
> -		new_len = sizeof(struct qllc);
> -		qllcptr = kzalloc(new_len, gfp_type() | GFP_DMA);
> -		if (qllcptr == NULL) {
> -			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
> -				"%s(%s): qllcptr allocation error",
> -						CTCM_FUNTAIL, dev->name);
> -			return -ENOMEM;
> -		}
> -
> -		qllcptr->qllc_address = 0xcc;
> -		qllcptr->qllc_commands = 0x03;
> -
> -		skb = __dev_alloc_skb(new_len, GFP_ATOMIC);
>  
> +		skb = __dev_alloc_skb(sizeof(struct qllc), GFP_ATOMIC);
>  		if (skb == NULL) {
>  			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
>  				"%s(%s): skb allocation error",
>  						CTCM_FUNTAIL, dev->name);
>  			priv->stats.rx_dropped++;
> -			kfree(qllcptr);
>  			return -ENOMEM;
>  		}
>  
> -		skb_put_data(skb, qllcptr, new_len);
> -		kfree(qllcptr);
> +		qllc.qllc_address = 0xcc;
> +		qllc.qllc_commands = 0x03;
> +
> +		skb_put_data(skb, &qllc, sizeof(struct qllc));
>  
>  		if (skb_headroom(skb) < 4) {
>  			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
> 

Re: [PATCH 0/6] s390/ctcm: Remove gfp_type() usage.

[Julian Wiedmann]

On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> Folks,
> 
> in the discussion about preempt count consistency across kernel
> configurations:
>   https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
> 
> Linus clearly requested that code in drivers and libraries which changes
> behaviour based on execution context should either be split up so that
> e.g. task context invocations and BH invocations have different interfaces
> or if that's not possible the context information has to be provided by the
> caller which knows in which context it is executing.
> 
> This series is removing the gfp_type() usage which uses in_interrupt().
> Overall it appears to be a nice cleanup according to the diffstat.

Thank you for digging through this mess!

> There are still two in_irq() users left in ctcm which I'm currently
> struggling with.
> 

If that fixes the sparse complaints about unbalanced locking, it will be
very much appreciated!

> Sebastian
> 
> 

Re: [PATCH 1/6] s390/ctcm: Put struct th_header and th_sweep on stack.

[Sebastian Andrzej Siewior]

On 2020-11-19 09:45:08 [+0200], Julian Wiedmann wrote:
> On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
> > The size of struct th_header is 8 byte and the size of struct th_sweep
> > is 16 byte. The memory for is allocated, initialized, used and
> > deallocated a few lines later.
> > 
> > It is more efficient to avoid the allocation/free dance and keeping the
> > variable on stack. Especially since the compiler is smart enough to not
> > allocate the memory on stack but assign the values directly.
> > 
> > Declare struct th_sweep/th_header on stack and initialize it to zero.
> > Use the local variable instead of the pointer.
> > 
> 
> Frankly, I'd much rather see us use the pointers that are returned from
> skb_push() and skb_put(). No need for the on-stack & memcpy indirection.

You are aware that the compiler optimizes the on-stack memory away and
you get the zero-init for free due to the way the assignment is made?
There is no memcpy() in the resulting code.

Sebastian

Re: [PATCH 1/6] s390/ctcm: Put struct th_header and th_sweep on stack.

[Julian Wiedmann]

On 19.11.20 10:12, Sebastian Andrzej Siewior wrote:
> On 2020-11-19 09:45:08 [+0200], Julian Wiedmann wrote:
>> On 18.11.20 12:53, Sebastian Andrzej Siewior wrote:
>>> The size of struct th_header is 8 byte and the size of struct th_sweep
>>> is 16 byte. The memory for is allocated, initialized, used and
>>> deallocated a few lines later.
>>>
>>> It is more efficient to avoid the allocation/free dance and keeping the
>>> variable on stack. Especially since the compiler is smart enough to not
>>> allocate the memory on stack but assign the values directly.
>>>
>>> Declare struct th_sweep/th_header on stack and initialize it to zero.
>>> Use the local variable instead of the pointer.
>>>
>>
>> Frankly, I'd much rather see us use the pointers that are returned from
>> skb_push() and skb_put(). No need for the on-stack & memcpy indirection.
> 
> You are aware that the compiler optimizes the on-stack memory away and
> you get the zero-init for free due to the way the assignment is made?
> There is no memcpy() in the resulting code.
> 
> Sebastian
> 

I am not worried about performance at all. Yet readability matters.

Re: [PATCH 1/6] s390/ctcm: Put struct th_header and th_sweep on stack.

[Sebastian Andrzej Siewior]

On 2020-11-19 10:16:28 [+0200], Julian Wiedmann wrote:
> I am not worried about performance at all. Yet readability matters.

As you wish. Let me respin it then.

Sebastian