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

[PATCH v2 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.

v1…v2:
	- Use the pointer from skb_put()/push() instead having it on
	  stack.
	- Collect acks for patch 4-6.

Sebastian

[PATCH v2 1/6] s390/ctcm: Avoid temporary allocation of struct th_header and th_sweep.

[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 assign the
values directly to skb's data part instead of using memcpy() for it.

Avoid an allocation of struct th_sweep/th_header and use the resulting
skb pointer instead.

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

diff --git a/drivers/s390/net/ctcm_fsms.c b/drivers/s390/net/ctcm_fsms.c
index 661d2a49bce96..b341075397d94 100644
--- a/drivers/s390/net/ctcm_fsms.c
+++ b/drivers/s390/net/ctcm_fsms.c
@@ -1303,12 +1303,10 @@ 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 = skb_push(ch->trans_skb, TH_HEADER_LENGTH);
+	memset(header, 0, TH_HEADER_LENGTH);
+
 	header->th_ch_flag = TH_HAS_PDU;  /* Normal data */
 	ch->th_seq_num++;
 	header->th_seq_num = ch->th_seq_num;
@@ -1316,11 +1314,6 @@ static void ctcmpc_chx_txdone(fsm_instance *fi, int event, void *arg)
 	CTCM_PR_DBGDATA("%s: ToVTAM_th_seq= %08x\n" ,
 					__func__, ch->th_seq_num);
 
-	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..a3a74ebfee635 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -623,25 +623,11 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
 				goto nomem;
 	}
 
-	header = kmalloc(TH_SWEEP_LENGTH, gfp_type());
-
-	if (!header) {
-		dev_kfree_skb_any(sweep_skb);
-		/* rc = -ENOMEM; */
-				goto nomem;
-	}
-
-	header->th.th_seg	= 0x00 ;
+	header = skb_put(sweep_skb, TH_SWEEP_LENGTH);
+	memset(header, 0, TH_SWEEP_LENGTH);
 	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);
-
 	netif_trans_update(dev);
 	skb_queue_tail(&ch->sweep_queue, sweep_skb);
 
@@ -768,25 +754,17 @@ 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;
+	/* put the TH on the packet */
+	header = skb_push(skb, TH_HEADER_LENGTH);
+	memset(header, 0, TH_HEADER_LENGTH);
 
-	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 */
 	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);
-
 	CTCM_PR_DBGDATA("%s(%s): skb len: %04x\n - pdu header and data for "
 			"up to 32 bytes sent to vtam:\n",
 				__func__, dev->name, skb->len);
diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 85a1a4533cbeb..0929ff2fd5c1a 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -655,24 +655,11 @@ static void ctcmpc_send_sweep_resp(struct channel *rch)
 		goto done;
 	}
 
-	header = kmalloc(sizeof(struct th_sweep), gfp_type());
-
-	if (!header) {
-		dev_kfree_skb_any(sweep_skb);
-		goto done;
-	}
-
-	header->th.th_seg	= 0x00 ;
+	header = skb_put(sweep_skb, TH_SWEEP_LENGTH);
+	memset(header, 0, TH_SWEEP_LENGTH);
 	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);
-
 	netif_trans_update(dev);
 	skb_queue_tail(&ch->sweep_queue, sweep_skb);
 
-- 
2.29.2

[PATCH v2 2/6] s390/ctcm: Avoid temporary allocation of struct qllc.

[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 assign the
values directly to skb's data part instead of using memcpy() for it.

Avoid an allocation of struct qllc and use the resulting skb pointer instead.

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

diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
index 0929ff2fd5c1a..76a9e0d0c0710 100644
--- a/drivers/s390/net/ctcm_mpc.c
+++ b/drivers/s390/net/ctcm_mpc.c
@@ -2049,7 +2049,6 @@ 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;
@@ -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);
+
+		qllcptr = skb_put(skb, sizeof(struct qllc));
+		qllcptr->qllc_address = 0xcc;
+		qllcptr->qllc_commands = 0x03;
 
 		if (skb_headroom(skb) < 4) {
 			CTCM_DBF_TEXT_(MPC_ERROR, CTC_DBF_ERROR,
-- 
2.29.2

[PATCH v2 3/6] s390/ctcm: Avoid temporary allocation of struct pdu.

[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 assign the
values directly to skb's data part instead of using memcpy() for it.

Avoid an allocation of struct pdu and use the resulting skb pointer instead.

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

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index a3a74ebfee635..0cb130c280031 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -666,24 +666,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 = skb_push(skb, PDU_HEADER_LENGTH);
 		p_header->pdu_offset = skb->len;
 		p_header->pdu_proto = 0x01;
-		p_header->pdu_flag = 0x00;
 		if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
-			p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
+			p_header->pdu_flag = PDU_FIRST | PDU_CNTL;
 		} else {
-			p_header->pdu_flag |= PDU_FIRST;
+			p_header->pdu_flag = PDU_FIRST;
 		}
 		p_header->pdu_seq = 0;
-		memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header,
-		       PDU_HEADER_LENGTH);
 
 		CTCM_PR_DEBUG("%s(%s): Put on collect_q - skb len: %04x \n"
 				"pdu header and data for up to 32 bytes:\n",
@@ -692,7 +684,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 +713,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 = skb_push(skb, PDU_HEADER_LENGTH);
 	p_header->pdu_offset = skb->len;
 	p_header->pdu_proto = 0x01;
-	p_header->pdu_flag = 0x00;
 	p_header->pdu_seq = 0;
 	if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
-		p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
+		p_header->pdu_flag = PDU_FIRST | PDU_CNTL;
 	} else {
-		p_header->pdu_flag |= PDU_FIRST;
+		p_header->pdu_flag = PDU_FIRST;
 	}
-	memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header, PDU_HEADER_LENGTH);
-
-	kfree(p_header);
 
 	if (ch->collect_len > 0) {
 		spin_lock_irqsave(&ch->collect_lock, saveflags);
-- 
2.29.2

[PATCH v2 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().

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>
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 76a9e0d0c0710..829deeedf21a2 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 v2 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.

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>
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 0cb130c280031..88efffe81c5ff 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1322,7 +1322,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 v2 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.

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>
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 88efffe81c5ff..59e491b344fa9 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -904,7 +904,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 v2 1/6] s390/ctcm: Avoid temporary allocation of struct th_header and th_sweep.

[Julian Wiedmann]

On 19.11.20 15:57, 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 assign the
> values directly to skb's data part instead of using memcpy() for it.
> 
> Avoid an allocation of struct th_sweep/th_header and use the resulting
> skb pointer instead.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/s390/net/ctcm_fsms.c | 15 ++++-----------
>  drivers/s390/net/ctcm_main.c | 32 +++++---------------------------
>  drivers/s390/net/ctcm_mpc.c  | 17 ++---------------
>  3 files changed, 11 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/s390/net/ctcm_fsms.c b/drivers/s390/net/ctcm_fsms.c
> index 661d2a49bce96..b341075397d94 100644
> --- a/drivers/s390/net/ctcm_fsms.c
> +++ b/drivers/s390/net/ctcm_fsms.c
> @@ -1303,12 +1303,10 @@ 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 = skb_push(ch->trans_skb, TH_HEADER_LENGTH);
> +	memset(header, 0, TH_HEADER_LENGTH);
> +
>  	header->th_ch_flag = TH_HAS_PDU;  /* Normal data */
>  	ch->th_seq_num++;
>  	header->th_seq_num = ch->th_seq_num;
> @@ -1316,11 +1314,6 @@ static void ctcmpc_chx_txdone(fsm_instance *fi, int event, void *arg)
>  	CTCM_PR_DBGDATA("%s: ToVTAM_th_seq= %08x\n" ,
>  					__func__, ch->th_seq_num);
>  
> -	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..a3a74ebfee635 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -623,25 +623,11 @@ static void ctcmpc_send_sweep_req(struct channel *rch)
>  				goto nomem;
>  	}
>  
> -	header = kmalloc(TH_SWEEP_LENGTH, gfp_type());
> -
> -	if (!header) {
> -		dev_kfree_skb_any(sweep_skb);
> -		/* rc = -ENOMEM; */
> -				goto nomem;
> -	}
> -
> -	header->th.th_seg	= 0x00 ;
> +	header = skb_put(sweep_skb, TH_SWEEP_LENGTH);
> +	memset(header, 0, TH_SWEEP_LENGTH);

Will squash this (and the one below) into skb_put_zero() when applying.

>  	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);
> -
>  	netif_trans_update(dev);
>  	skb_queue_tail(&ch->sweep_queue, sweep_skb);
>  
> @@ -768,25 +754,17 @@ 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;
> +	/* put the TH on the packet */
> +	header = skb_push(skb, TH_HEADER_LENGTH);
> +	memset(header, 0, TH_HEADER_LENGTH);
>  
> -	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 */
>  	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);
> -
>  	CTCM_PR_DBGDATA("%s(%s): skb len: %04x\n - pdu header and data for "
>  			"up to 32 bytes sent to vtam:\n",
>  				__func__, dev->name, skb->len);
> diff --git a/drivers/s390/net/ctcm_mpc.c b/drivers/s390/net/ctcm_mpc.c
> index 85a1a4533cbeb..0929ff2fd5c1a 100644
> --- a/drivers/s390/net/ctcm_mpc.c
> +++ b/drivers/s390/net/ctcm_mpc.c
> @@ -655,24 +655,11 @@ static void ctcmpc_send_sweep_resp(struct channel *rch)
>  		goto done;
>  	}
>  
> -	header = kmalloc(sizeof(struct th_sweep), gfp_type());
> -
> -	if (!header) {
> -		dev_kfree_skb_any(sweep_skb);
> -		goto done;
> -	}
> -
> -	header->th.th_seg	= 0x00 ;
> +	header = skb_put(sweep_skb, TH_SWEEP_LENGTH);
> +	memset(header, 0, TH_SWEEP_LENGTH);
>  	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);
> -
>  	netif_trans_update(dev);
>  	skb_queue_tail(&ch->sweep_queue, sweep_skb);
>  
> 

Re: [PATCH v2 3/6] s390/ctcm: Avoid temporary allocation of struct pdu.

[Julian Wiedmann]

On 19.11.20 15:57, 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 assign the
> values directly to skb's data part instead of using memcpy() for it.
> 
> Avoid an allocation of struct pdu and use the resulting skb pointer instead.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/s390/net/ctcm_main.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index a3a74ebfee635..0cb130c280031 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -666,24 +666,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 = skb_push(skb, PDU_HEADER_LENGTH);
>  		p_header->pdu_offset = skb->len;

I mentioned this in my reply to v1 - here we now need to adjust skb->len
for the pushed length. Will fix up while applying (also below).

>  		p_header->pdu_proto = 0x01;
> -		p_header->pdu_flag = 0x00;
>  		if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
> -			p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
> +			p_header->pdu_flag = PDU_FIRST | PDU_CNTL;
>  		} else {
> -			p_header->pdu_flag |= PDU_FIRST;
> +			p_header->pdu_flag = PDU_FIRST;
>  		}
>  		p_header->pdu_seq = 0;
> -		memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header,
> -		       PDU_HEADER_LENGTH);
>  
>  		CTCM_PR_DEBUG("%s(%s): Put on collect_q - skb len: %04x \n"
>  				"pdu header and data for up to 32 bytes:\n",
> @@ -692,7 +684,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 +713,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 = skb_push(skb, PDU_HEADER_LENGTH);
>  	p_header->pdu_offset = skb->len;
>  	p_header->pdu_proto = 0x01;
> -	p_header->pdu_flag = 0x00;
>  	p_header->pdu_seq = 0;
>  	if (be16_to_cpu(skb->protocol) == ETH_P_SNAP) {
> -		p_header->pdu_flag |= PDU_FIRST | PDU_CNTL;
> +		p_header->pdu_flag = PDU_FIRST | PDU_CNTL;
>  	} else {
> -		p_header->pdu_flag |= PDU_FIRST;
> +		p_header->pdu_flag = PDU_FIRST;
>  	}
> -	memcpy(skb_push(skb, PDU_HEADER_LENGTH), p_header, PDU_HEADER_LENGTH);
> -
> -	kfree(p_header);
>  
>  	if (ch->collect_len > 0) {
>  		spin_lock_irqsave(&ch->collect_lock, saveflags);
> 

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

[Julian Wiedmann]

On 19.11.20 15:57, 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.
> 
> v1…v2:
> 	- Use the pointer from skb_put()/push() instead having it on
> 	  stack.
> 	- Collect acks for patch 4-6.
> 
> Sebastian
> 
> 

Thank you Sebastian! All applied.

Re: [PATCH v2 3/6] s390/ctcm: Avoid temporary allocation of struct pdu.

[Sebastian Andrzej Siewior]

On 2020-11-20 10:05:45 [+0200], Julian Wiedmann wrote:
> > diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> > index a3a74ebfee635..0cb130c280031 100644
> > --- a/drivers/s390/net/ctcm_main.c
> > +++ b/drivers/s390/net/ctcm_main.c
> > @@ -666,24 +666,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 = skb_push(skb, PDU_HEADER_LENGTH);
> >  		p_header->pdu_offset = skb->len;
> 
> I mentioned this in my reply to v1 - here we now need to adjust skb->len
> for the pushed length. Will fix up while applying (also below).

I'm sorry. It took me a while to understand what you meant and then I
forgot about it while I was here.
Thank you for taking care of it.

Sebastian