RE: [PATCH net-next] net: mana: Add support for jumbo frame

[Haiyang Zhang]

> -----Original Message-----
> From: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> Sent: Monday, March 20, 2023 5:42 AM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx
> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> Paul Rosswurm <paulros@xxxxxxxxxxxxx>; olaf@xxxxxxxxx;
> vkuznets@xxxxxxxxxx; davem@xxxxxxxxxxxxx; wei.liu@xxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> leon@xxxxxxxxxx; Long Li <longli@xxxxxxxxxxxxx>;
> ssengar@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: mana: Add support for jumbo frame
> 
> On 2023/3/20 5:27, Haiyang Zhang wrote:
> > During probe, get the hardware allowed max MTU by querying the device
> > configuration. Users can select MTU up to the device limit. Also,
> > when XDP is in use, we currently limit the buffer size to one page.
> >
> > Updated RX data path to allocate and use RX queue DMA buffers with
> > proper size based on the MTU setting.
> 
> The change in this patch seems better to splitted into more reviewable
> patchset. Perhaps refactor the RX queue DMA buffers allocation to handle
> different size first, then add support for the jumbo frame.

Will consider.

> 
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > ---
> >  .../net/ethernet/microsoft/mana/mana_bpf.c    |  22 +-
> >  drivers/net/ethernet/microsoft/mana/mana_en.c | 229 ++++++++++++----
> --
> >  include/net/mana/gdma.h                       |   4 +
> >  include/net/mana/mana.h                       |  18 +-
> >  4 files changed, 183 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> > index 3caea631229c..23b1521c0df9 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> > @@ -133,12 +133,6 @@ u32 mana_run_xdp(struct net_device *ndev,
> struct mana_rxq *rxq,
> >  	return act;
> >  }
> >
> > -static unsigned int mana_xdp_fraglen(unsigned int len)
> > -{
> > -	return SKB_DATA_ALIGN(len) +
> > -	       SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > -}
> > -
> >  struct bpf_prog *mana_xdp_get(struct mana_port_context *apc)
> >  {
> >  	ASSERT_RTNL();
> > @@ -179,17 +173,18 @@ static int mana_xdp_set(struct net_device *ndev,
> struct bpf_prog *prog,
> >  {
> >  	struct mana_port_context *apc = netdev_priv(ndev);
> >  	struct bpf_prog *old_prog;
> > -	int buf_max;
> > +	struct gdma_context *gc;
> > +
> > +	gc = apc->ac->gdma_dev->gdma_context;
> >
> >  	old_prog = mana_xdp_get(apc);
> >
> >  	if (!old_prog && !prog)
> >  		return 0;
> >
> > -	buf_max = XDP_PACKET_HEADROOM + mana_xdp_fraglen(ndev-
> >mtu + ETH_HLEN);
> > -	if (prog && buf_max > PAGE_SIZE) {
> > -		netdev_err(ndev, "XDP: mtu:%u too large, buf_max:%u\n",
> > -			   ndev->mtu, buf_max);
> > +	if (prog && ndev->mtu > MANA_XDP_MTU_MAX) {
> > +		netdev_err(ndev, "XDP: mtu:%u too large, mtu_max:%lu\n",
> > +			   ndev->mtu, MANA_XDP_MTU_MAX);
> >  		NL_SET_ERR_MSG_MOD(extack, "XDP: mtu too large");
> >
> >  		return -EOPNOTSUPP;
> > @@ -206,6 +201,11 @@ static int mana_xdp_set(struct net_device *ndev,
> struct bpf_prog *prog,
> >  	if (apc->port_is_up)
> >  		mana_chn_setxdp(apc, prog);
> >
> > +	if (prog)
> > +		ndev->max_mtu = MANA_XDP_MTU_MAX;
> > +	else
> > +		ndev->max_mtu = gc->adapter_mtu - ETH_HLEN;
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 492474b4d8aa..07738b7e85f2 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -427,6 +427,34 @@ static u16 mana_select_queue(struct net_device
> *ndev, struct sk_buff *skb,
> >  	return txq;
> >  }
> >
> > +static int mana_change_mtu(struct net_device *ndev, int new_mtu)
> > +{
> > +	unsigned int old_mtu = ndev->mtu;
> > +	int err, err2;
> > +
> > +	err = mana_detach(ndev, false);
> > +	if (err) {
> > +		netdev_err(ndev, "mana_detach failed: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	ndev->mtu = new_mtu;
> > +
> > +	err = mana_attach(ndev);
> > +	if (!err)
> > +		return 0;
> > +
> > +	netdev_err(ndev, "mana_attach failed: %d\n", err);
> > +
> > +	/* Try to roll it back to the old configuration. */
> > +	ndev->mtu = old_mtu;
> > +	err2 = mana_attach(ndev);
> > +	if (err2)
> > +		netdev_err(ndev, "mana re-attach failed: %d\n", err2);
> > +
> > +	return err;
> > +}
> > +
> >  static const struct net_device_ops mana_devops = {
> >  	.ndo_open		= mana_open,
> >  	.ndo_stop		= mana_close,
> > @@ -436,6 +464,7 @@ static const struct net_device_ops mana_devops = {
> >  	.ndo_get_stats64	= mana_get_stats64,
> >  	.ndo_bpf		= mana_bpf,
> >  	.ndo_xdp_xmit		= mana_xdp_xmit,
> > +	.ndo_change_mtu		= mana_change_mtu,
> >  };
> >
> >  static void mana_cleanup_port_context(struct mana_port_context *apc)
> > @@ -625,6 +654,9 @@ static int mana_query_device_cfg(struct
> mana_context *ac, u32 proto_major_ver,
> >
> >  	mana_gd_init_req_hdr(&req.hdr, MANA_QUERY_DEV_CONFIG,
> >  			     sizeof(req), sizeof(resp));
> > +
> > +	req.hdr.resp.msg_version = GDMA_MESSAGE_V2;
> 
> hdr->req.msg_version and hdr->resp.msg_version are both set to
> GDMA_MESSAGE_V1 in mana_gd_init_req_hdr(), is there any reason
> why hdr->req.msg_version is not set to GDMA_MESSAGE_V2?

The request version is still V1 in our hardware.

> Does initializing req.hdr.resp.msg_version to GDMA_MESSAGE_V2
> in mana_gd_init_req_hdr() without reset it to GDMA_MESSAGE_V2
> in mana_query_device_cfg() affect other user?

Yes, other users still need V1. So only this message response version is set 
to V2.

> 
> 
> > +
> >  	req.proto_major_ver = proto_major_ver;
> >  	req.proto_minor_ver = proto_minor_ver;
> >  	req.proto_micro_ver = proto_micro_ver;
> > @@ -647,6 +679,11 @@ static int mana_query_device_cfg(struct
> mana_context *ac, u32 proto_major_ver,
> >
> >  	*max_num_vports = resp.max_num_vports;
> >
> > +	if (resp.hdr.response.msg_version == GDMA_MESSAGE_V2)
> 
> It seems the driver is setting resp.hdr.response.msg_version to
> GDMA_MESSAGE_V2 above, and do the checking here. Does older
> firmware reset the resp.hdr.response.msg_version to GDMA_MESSAGE_V1
> in order to enable compatibility between firmware and driver?

Yes older firmware still using V1.

> 
> > +		gc->adapter_mtu = resp.adapter_mtu;
> > +	else
> > +		gc->adapter_mtu = ETH_FRAME_LEN;
> > +
> >  	return 0;
> >  }
> >
> > @@ -1185,10 +1222,10 @@ static void mana_post_pkt_rxq(struct
> mana_rxq *rxq)
> >  	WARN_ON_ONCE(recv_buf_oob->wqe_inf.wqe_size_in_bu != 1);
> >  }
> >
> > -static struct sk_buff *mana_build_skb(void *buf_va, uint pkt_len,
> > -				      struct xdp_buff *xdp)
> > +static struct sk_buff *mana_build_skb(struct mana_rxq *rxq, void *buf_va,
> > +				      uint pkt_len, struct xdp_buff *xdp)
> >  {
> > -	struct sk_buff *skb = build_skb(buf_va, PAGE_SIZE);
> > +	struct sk_buff *skb = napi_build_skb(buf_va, rxq->alloc_size);
> 
> Changing build_skb() to napi_build_skb() seems like an optimization
> unrelated to jumbo frame support, seems like another patch to do that?
Will do. Thanks.

- Haiyang