* [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet()
@ 2022-07-26 19:56 Bob Pearson
2022-07-27 10:23 ` Haris Iqbal
0 siblings, 1 reply; 4+ messages in thread
From: Bob Pearson @ 2022-07-26 19:56 UTC (permalink / raw)
To: jgg, zyjzyj2000, linux-rdma; +Cc: Bob Pearson
Currently rxe_init_packet() recomputes ndev from the address
vector but struct rxe_dev already holds a reference to ndev.
Cleanup rxe_init_packet to use the value of ndev in rxe and drop
an unneeded parameter paylen since it is already carried in the
packet info struct.
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
drivers/infiniband/sw/rxe/rxe_loc.h | 2 +-
drivers/infiniband/sw/rxe/rxe_net.c | 49 +++++++++++-----------------
drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
drivers/infiniband/sw/rxe/rxe_resp.c | 3 +-
4 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
index f9af30f582c6..7f98d296bc0d 100644
--- a/drivers/infiniband/sw/rxe/rxe_loc.h
+++ b/drivers/infiniband/sw/rxe/rxe_loc.h
@@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
/* rxe_net.c */
struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
- int paylen, struct rxe_pkt_info *pkt);
+ struct rxe_pkt_info *pkt);
int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
struct sk_buff *skb);
int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index c53f4529f098..4a091f236dde 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
return err;
}
+/**
+ * rxe_init_packet - allocate and initialize a new skb
+ * @rxe: rxe device
+ * @av: remote address vector
+ * @pkt: packet info
+ *
+ * Returns: an skb on success else NULL
+ */
struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
- int paylen, struct rxe_pkt_info *pkt)
+ struct rxe_pkt_info *pkt)
{
unsigned int hdr_len;
struct sk_buff *skb = NULL;
- struct net_device *ndev;
- const struct ib_gid_attr *attr;
+ struct net_device *ndev = rxe->ndev;
const int port_num = 1;
-
- attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
- if (IS_ERR(attr))
- return NULL;
+ int skb_size;
if (av->network_type == RXE_NETWORK_TYPE_IPV4)
hdr_len = ETH_HLEN + sizeof(struct udphdr) +
@@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
hdr_len = ETH_HLEN + sizeof(struct udphdr) +
sizeof(struct ipv6hdr);
- rcu_read_lock();
- ndev = rdma_read_gid_attr_ndev_rcu(attr);
- if (IS_ERR(ndev)) {
- rcu_read_unlock();
- goto out;
- }
- skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
- GFP_ATOMIC);
-
- if (unlikely(!skb)) {
- rcu_read_unlock();
- goto out;
- }
-
- skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
-
- /* FIXME: hold reference to this netdev until life of this skb. */
- skb->dev = ndev;
- rcu_read_unlock();
+ skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
+ skb = alloc_skb(skb_size, GFP_ATOMIC);
+ if (unlikely(!skb))
+ return NULL;
+ skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
+ skb->dev = ndev;
if (av->network_type == RXE_NETWORK_TYPE_IPV4)
skb->protocol = htons(ETH_P_IP);
else
@@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
pkt->rxe = rxe;
pkt->port_num = port_num;
- pkt->hdr = skb_put(skb, paylen);
- pkt->mask |= RXE_GRH_MASK;
+ pkt->hdr = skb_put(skb, pkt->paylen);
+ pkt->mask |= RXE_GRH_MASK;
-out:
- rdma_put_gid_attr(attr);
return skb;
}
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 49e8f54db6f5..e72db960d7ea 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
pkt->paylen = paylen;
/* init skb */
- skb = rxe_init_packet(rxe, av, paylen, pkt);
+ skb = rxe_init_packet(rxe, av, pkt);
if (unlikely(!skb))
return NULL;
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index b36ec5c4d5e0..02a5d4ebb45e 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
*/
pad = (-payload) & 0x3;
paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
+ ack->paylen = paylen;
- skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
+ skb = rxe_init_packet(rxe, &qp->pri_av, ack);
if (!skb)
return NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet()
2022-07-26 19:56 [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet() Bob Pearson
@ 2022-07-27 10:23 ` Haris Iqbal
2022-07-27 16:01 ` Bob Pearson
0 siblings, 1 reply; 4+ messages in thread
From: Haris Iqbal @ 2022-07-27 10:23 UTC (permalink / raw)
To: Bob Pearson; +Cc: jgg, zyjzyj2000, linux-rdma
On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> Currently rxe_init_packet() recomputes ndev from the address
> vector but struct rxe_dev already holds a reference to ndev.
>
> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
> an unneeded parameter paylen since it is already carried in the
> packet info struct.
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> ---
> drivers/infiniband/sw/rxe/rxe_loc.h | 2 +-
> drivers/infiniband/sw/rxe/rxe_net.c | 49 +++++++++++-----------------
> drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
> drivers/infiniband/sw/rxe/rxe_resp.c | 3 +-
> 4 files changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index f9af30f582c6..7f98d296bc0d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
>
> /* rxe_net.c */
> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> - int paylen, struct rxe_pkt_info *pkt);
> + struct rxe_pkt_info *pkt);
> int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
> struct sk_buff *skb);
> int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c53f4529f098..4a091f236dde 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> return err;
> }
>
> +/**
> + * rxe_init_packet - allocate and initialize a new skb
> + * @rxe: rxe device
> + * @av: remote address vector
> + * @pkt: packet info
> + *
> + * Returns: an skb on success else NULL
> + */
> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> - int paylen, struct rxe_pkt_info *pkt)
> + struct rxe_pkt_info *pkt)
> {
> unsigned int hdr_len;
> struct sk_buff *skb = NULL;
> - struct net_device *ndev;
> - const struct ib_gid_attr *attr;
> + struct net_device *ndev = rxe->ndev;
> const int port_num = 1;
> -
> - attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
> - if (IS_ERR(attr))
> - return NULL;
> + int skb_size;
>
> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> sizeof(struct ipv6hdr);
>
> - rcu_read_lock();
Was removing this rcu lock intentional? If so, do we need a mention of
this in the commit message why its not needed anymore?
> - ndev = rdma_read_gid_attr_ndev_rcu(attr);
> - if (IS_ERR(ndev)) {
> - rcu_read_unlock();
> - goto out;
> - }
> - skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
> - GFP_ATOMIC);
> -
> - if (unlikely(!skb)) {
> - rcu_read_unlock();
> - goto out;
> - }
> -
> - skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
> -
> - /* FIXME: hold reference to this netdev until life of this skb. */
> - skb->dev = ndev;
> - rcu_read_unlock();
> + skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
> + skb = alloc_skb(skb_size, GFP_ATOMIC);
> + if (unlikely(!skb))
> + return NULL;
>
> + skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
> + skb->dev = ndev;
> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> skb->protocol = htons(ETH_P_IP);
> else
> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>
> pkt->rxe = rxe;
> pkt->port_num = port_num;
> - pkt->hdr = skb_put(skb, paylen);
> - pkt->mask |= RXE_GRH_MASK;
> + pkt->hdr = skb_put(skb, pkt->paylen);
> + pkt->mask |= RXE_GRH_MASK;
>
> -out:
> - rdma_put_gid_attr(attr);
> return skb;
> }
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 49e8f54db6f5..e72db960d7ea 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
> pkt->paylen = paylen;
>
> /* init skb */
> - skb = rxe_init_packet(rxe, av, paylen, pkt);
> + skb = rxe_init_packet(rxe, av, pkt);
> if (unlikely(!skb))
> return NULL;
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index b36ec5c4d5e0..02a5d4ebb45e 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
> */
> pad = (-payload) & 0x3;
> paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
> + ack->paylen = paylen;
Maybe remove the old ack->paylen assignment which is done later in
this function?
>
> - skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
> + skb = rxe_init_packet(rxe, &qp->pri_av, ack);
> if (!skb)
> return NULL;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet()
2022-07-27 10:23 ` Haris Iqbal
@ 2022-07-27 16:01 ` Bob Pearson
2022-07-27 16:34 ` Haris Iqbal
0 siblings, 1 reply; 4+ messages in thread
From: Bob Pearson @ 2022-07-27 16:01 UTC (permalink / raw)
To: Haris Iqbal; +Cc: jgg, zyjzyj2000, linux-rdma
On 7/27/22 05:23, Haris Iqbal wrote:
> On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> Currently rxe_init_packet() recomputes ndev from the address
>> vector but struct rxe_dev already holds a reference to ndev.
>>
>> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
>> an unneeded parameter paylen since it is already carried in the
>> packet info struct.
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>> ---
>> drivers/infiniband/sw/rxe/rxe_loc.h | 2 +-
>> drivers/infiniband/sw/rxe/rxe_net.c | 49 +++++++++++-----------------
>> drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
>> drivers/infiniband/sw/rxe/rxe_resp.c | 3 +-
>> 4 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
>> index f9af30f582c6..7f98d296bc0d 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
>> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
>> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
>>
>> /* rxe_net.c */
>> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>> - int paylen, struct rxe_pkt_info *pkt);
>> + struct rxe_pkt_info *pkt);
>> int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
>> struct sk_buff *skb);
>> int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
>> index c53f4529f098..4a091f236dde 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_net.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
>> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
>> return err;
>> }
>>
>> +/**
>> + * rxe_init_packet - allocate and initialize a new skb
>> + * @rxe: rxe device
>> + * @av: remote address vector
>> + * @pkt: packet info
>> + *
>> + * Returns: an skb on success else NULL
>> + */
>> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>> - int paylen, struct rxe_pkt_info *pkt)
>> + struct rxe_pkt_info *pkt)
>> {
>> unsigned int hdr_len;
>> struct sk_buff *skb = NULL;
>> - struct net_device *ndev;
>> - const struct ib_gid_attr *attr;
>> + struct net_device *ndev = rxe->ndev;
>> const int port_num = 1;
>> -
>> - attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
>> - if (IS_ERR(attr))
>> - return NULL;
>> + int skb_size;
>>
>> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
>> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
>> sizeof(struct ipv6hdr);
>>
>> - rcu_read_lock();
>
> Was removing this rcu lock intentional? If so, do we need a mention of
> this in the commit message why its not needed anymore?
>
The rcu lock was required to use the rdma_read_gid_attr_ndev_rcu() API.
For the rxe driver there is no way that the ndev is changing once the rxe device
is set up and ndev was passed in to rxe_newlink() and saved in the rxe_dev struct.
Not any good reason to do all this work to get ndev when we already know it.
>> - ndev = rdma_read_gid_attr_ndev_rcu(attr);
>> - if (IS_ERR(ndev)) {
>> - rcu_read_unlock();
>> - goto out;
>> - }
>> - skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
>> - GFP_ATOMIC);
>> -
>> - if (unlikely(!skb)) {
>> - rcu_read_unlock();
>> - goto out;
>> - }
>> -
>> - skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
>> -
>> - /* FIXME: hold reference to this netdev until life of this skb. */
>> - skb->dev = ndev;
>> - rcu_read_unlock();
>> + skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
>> + skb = alloc_skb(skb_size, GFP_ATOMIC);
>> + if (unlikely(!skb))
>> + return NULL;
>>
>> + skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
>> + skb->dev = ndev;
>> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
>> skb->protocol = htons(ETH_P_IP);
>> else
>> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
>>
>> pkt->rxe = rxe;
>> pkt->port_num = port_num;
>> - pkt->hdr = skb_put(skb, paylen);
>> - pkt->mask |= RXE_GRH_MASK;
>> + pkt->hdr = skb_put(skb, pkt->paylen);
>> + pkt->mask |= RXE_GRH_MASK;
>>
>> -out:
>> - rdma_put_gid_attr(attr);
>> return skb;
>> }
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
>> index 49e8f54db6f5..e72db960d7ea 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_req.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
>> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
>> pkt->paylen = paylen;
>>
>> /* init skb */
>> - skb = rxe_init_packet(rxe, av, paylen, pkt);
>> + skb = rxe_init_packet(rxe, av, pkt);
>> if (unlikely(!skb))
>> return NULL;
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index b36ec5c4d5e0..02a5d4ebb45e 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
>> */
>> pad = (-payload) & 0x3;
>> paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
>> + ack->paylen = paylen;
>
> Maybe remove the old ack->paylen assignment which is done later in
> this function?
>
Agreed. Will send a v2 with this change.
>>
>> - skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
>> + skb = rxe_init_packet(rxe, &qp->pri_av, ack);
>> if (!skb)
>> return NULL;
>>
>> --
>> 2.34.1
>>
Bob
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet()
2022-07-27 16:01 ` Bob Pearson
@ 2022-07-27 16:34 ` Haris Iqbal
0 siblings, 0 replies; 4+ messages in thread
From: Haris Iqbal @ 2022-07-27 16:34 UTC (permalink / raw)
To: Bob Pearson; +Cc: jgg, zyjzyj2000, linux-rdma
On Wed, Jul 27, 2022 at 6:01 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 7/27/22 05:23, Haris Iqbal wrote:
> > On Tue, Jul 26, 2022 at 9:56 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>
> >> Currently rxe_init_packet() recomputes ndev from the address
> >> vector but struct rxe_dev already holds a reference to ndev.
> >>
> >> Cleanup rxe_init_packet to use the value of ndev in rxe and drop
> >> an unneeded parameter paylen since it is already carried in the
> >> packet info struct.
> >>
> >> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
> >> ---
> >> drivers/infiniband/sw/rxe/rxe_loc.h | 2 +-
> >> drivers/infiniband/sw/rxe/rxe_net.c | 49 +++++++++++-----------------
> >> drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
> >> drivers/infiniband/sw/rxe/rxe_resp.c | 3 +-
> >> 4 files changed, 23 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> >> index f9af30f582c6..7f98d296bc0d 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> >> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> >> @@ -93,7 +93,7 @@ void rxe_mw_cleanup(struct rxe_pool_elem *elem);
> >>
> >> /* rxe_net.c */
> >> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >> - int paylen, struct rxe_pkt_info *pkt);
> >> + struct rxe_pkt_info *pkt);
> >> int rxe_prepare(struct rxe_av *av, struct rxe_pkt_info *pkt,
> >> struct sk_buff *skb);
> >> int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> >> index c53f4529f098..4a091f236dde 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> >> @@ -443,18 +443,22 @@ int rxe_xmit_packet(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
> >> return err;
> >> }
> >>
> >> +/**
> >> + * rxe_init_packet - allocate and initialize a new skb
> >> + * @rxe: rxe device
> >> + * @av: remote address vector
> >> + * @pkt: packet info
> >> + *
> >> + * Returns: an skb on success else NULL
> >> + */
> >> struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >> - int paylen, struct rxe_pkt_info *pkt)
> >> + struct rxe_pkt_info *pkt)
> >> {
> >> unsigned int hdr_len;
> >> struct sk_buff *skb = NULL;
> >> - struct net_device *ndev;
> >> - const struct ib_gid_attr *attr;
> >> + struct net_device *ndev = rxe->ndev;
> >> const int port_num = 1;
> >> -
> >> - attr = rdma_get_gid_attr(&rxe->ib_dev, port_num, av->grh.sgid_index);
> >> - if (IS_ERR(attr))
> >> - return NULL;
> >> + int skb_size;
> >>
> >> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> >> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> >> @@ -463,26 +467,13 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >> hdr_len = ETH_HLEN + sizeof(struct udphdr) +
> >> sizeof(struct ipv6hdr);
> >>
> >> - rcu_read_lock();
> >
> > Was removing this rcu lock intentional? If so, do we need a mention of
> > this in the commit message why its not needed anymore?
> >
> The rcu lock was required to use the rdma_read_gid_attr_ndev_rcu() API.
> For the rxe driver there is no way that the ndev is changing once the rxe device
> is set up and ndev was passed in to rxe_newlink() and saved in the rxe_dev struct.
> Not any good reason to do all this work to get ndev when we already know it.
I see. Thanks for the explanation.
One more question for my understanding.
There was a FIXME line (removed with this commit) mentioning that we
should hold a reference to the netdev. I assume it was talking about
dev_hold.
Now, while initing the rxe we call ib_device_set_netdev, which does
dev_hold. But I could not find rxe doing dev_hold anywhere directly. I
am assuming this is enough, or should rxe be also holding a reference
for this net device since technically it does use it directly at some
places?
> >> - ndev = rdma_read_gid_attr_ndev_rcu(attr);
> >> - if (IS_ERR(ndev)) {
> >> - rcu_read_unlock();
> >> - goto out;
> >> - }
> >> - skb = alloc_skb(paylen + hdr_len + LL_RESERVED_SPACE(ndev),
> >> - GFP_ATOMIC);
> >> -
> >> - if (unlikely(!skb)) {
> >> - rcu_read_unlock();
> >> - goto out;
> >> - }
> >> -
> >> - skb_reserve(skb, hdr_len + LL_RESERVED_SPACE(ndev));
> >> -
> >> - /* FIXME: hold reference to this netdev until life of this skb. */
> >> - skb->dev = ndev;
> >> - rcu_read_unlock();
> >> + skb_size = LL_RESERVED_SPACE(ndev) + hdr_len + pkt->paylen;
> >> + skb = alloc_skb(skb_size, GFP_ATOMIC);
> >> + if (unlikely(!skb))
> >> + return NULL;
> >>
> >> + skb_reserve(skb, LL_RESERVED_SPACE(ndev) + hdr_len);
> >> + skb->dev = ndev;
> >> if (av->network_type == RXE_NETWORK_TYPE_IPV4)
> >> skb->protocol = htons(ETH_P_IP);
> >> else
> >> @@ -490,11 +481,9 @@ struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> >>
> >> pkt->rxe = rxe;
> >> pkt->port_num = port_num;
> >> - pkt->hdr = skb_put(skb, paylen);
> >> - pkt->mask |= RXE_GRH_MASK;
> >> + pkt->hdr = skb_put(skb, pkt->paylen);
> >> + pkt->mask |= RXE_GRH_MASK;
> >>
> >> -out:
> >> - rdma_put_gid_attr(attr);
> >> return skb;
> >> }
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> >> index 49e8f54db6f5..e72db960d7ea 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> >> @@ -397,7 +397,7 @@ static struct sk_buff *init_req_packet(struct rxe_qp *qp,
> >> pkt->paylen = paylen;
> >>
> >> /* init skb */
> >> - skb = rxe_init_packet(rxe, av, paylen, pkt);
> >> + skb = rxe_init_packet(rxe, av, pkt);
> >> if (unlikely(!skb))
> >> return NULL;
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> index b36ec5c4d5e0..02a5d4ebb45e 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> >> @@ -670,8 +670,9 @@ static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
> >> */
> >> pad = (-payload) & 0x3;
> >> paylen = rxe_opcode[opcode].length + payload + pad + RXE_ICRC_SIZE;
> >> + ack->paylen = paylen;
> >
> > Maybe remove the old ack->paylen assignment which is done later in
> > this function?
> >
> Agreed. Will send a v2 with this change.
> >>
> >> - skb = rxe_init_packet(rxe, &qp->pri_av, paylen, ack);
> >> + skb = rxe_init_packet(rxe, &qp->pri_av, ack);
> >> if (!skb)
> >> return NULL;
> >>
> >> --
> >> 2.34.1
> >>
>
> Bob
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-27 16:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-26 19:56 [PATCH for-next] RDMA/rxe: Cleanup rxe_init_packet() Bob Pearson
2022-07-27 10:23 ` Haris Iqbal
2022-07-27 16:01 ` Bob Pearson
2022-07-27 16:34 ` Haris Iqbal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.