Re: [PATCH for-next v3 2/2] RDMA/rxe: Call rxe_set_mtu after rxe_register_device

[Zhu Yanjun]

在 2023/9/19 11:25, Zhijian Li (Fujitsu) 写道:
> On 19/09/2023 09:11, Zhu Yanjun wrote:
> > On Tue, Sep 19, 2023 at 8:57 AM Zhijian Li (Fujitsu)
> > <lizhijian@xxxxxxxxxxx> wrote:
> > > On 18/09/2023 20:37, Leon Romanovsky wrote:
> > > > On Mon, Sep 18, 2023 at 10:05:43AM +0800, Li Zhijian wrote:
> > > > > rxe_set_mtu() will call rxe_info_dev() to print message, and
> > > > > rxe_info_dev() expects dev_name(rxe->ib_dev->dev) has been assigned.
> > > > >
> > > > > Previously since dev_name() is not set, when a new rxe link is being
> > > > > added, 'null' will be used as the dev_name like:
> > > > >
> > > > > "(null): rxe_set_mtu: Set mtu to 1024"
> > > > >
> > > > > Move rxe_register_device() earlier to assign the correct dev_name
> > > > > so that it can be read by rxe_set_mtu() later.
> > > > I would expect removal of that print line instead of moving
> > > > rxe_register_device().
> > >
> > > I also struggled with this point. The last option is keep it as it is.
> > > Once rxe is registered, this print will work fine.
> > I delved into the source code. About moving rxe_register_device, I
> > could not find any harm to the driver.
> The point i'm struggling was that, it's strange/opaque to move rxe_register_device().
> There is no doubt that the original order was more clear.


Please check the source code. It is very clear.


> In terms of the message content, is it valuable to print(pr_info) this message, i noticed
> that there is a duplicate pr_dbg() in rxe_notify().


rxe_notify is for netdevice event, not for rdma event.


> rxe's mtu is always same with the NIC, isn't it ?

Please check the following source code.

void rxe_set_mtu(struct rxe_dev *rxe, unsigned int ndev_mtu)

{
        struct rxe_port *port = &rxe->port;
        enum ib_mtu mtu;

        mtu = eth_mtu_int_to_enum(ndev_mtu);

        /* Make sure that new MTU in range */
        mtu = mtu ? min_t(enum ib_mtu, mtu, IB_MTU_4096) : IB_MTU_256;

        port->attr.active_mtu = mtu;
        port->mtu_cap = ib_mtu_enum_to_int(mtu);

        rxe_info_dev(rxe, "Set mtu to %d", port->mtu_cap);
}

Zhu Yanjun


> Thanks
> Zhijian
>
>
>
> > So I think this is also a solution to this problem.
> >
> > Zhu Yanjun
> >
> > > Thanks
> > > Zhijian
> > >
> > >
> > > > Thanks
> > > >
> > > > > And it's safe to do such change since mtu will not be used during the
> > > > > rxe_register_device()
> > > > >
> > > > > After this change, the message becomes:
> > > > > "rxe_eth0: rxe_set_mtu: Set mtu to 4096"
> > > > >
> > > > > Fixes: 9ac01f434a1e ("RDMA/rxe: Extend dbg log messages to err and info")
> > > > > Reviewed-by: Daisuke Matsuda <matsuda-daisuke@xxxxxxxxxxx>
> > > > > Reviewed-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
> > > > > Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
> > > > > ---
> > > > >     drivers/infiniband/sw/rxe/rxe.c | 5 ++++-
> > > > >     1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> > > > > index a086d588e159..8a43c0c4f8d8 100644
> > > > > --- a/drivers/infiniband/sw/rxe/rxe.c
> > > > > +++ b/drivers/infiniband/sw/rxe/rxe.c
> > > > > @@ -169,10 +169,13 @@ void rxe_set_mtu(struct rxe_dev *rxe, unsigned int ndev_mtu)
> > > > >      */
> > > > >     int rxe_add(struct rxe_dev *rxe, unsigned int mtu, const char *ibdev_name)
> > > > >     {
> > > > > +    int ret;
> > > > > +
> > > > >        rxe_init(rxe);
> > > > > +    ret = rxe_register_device(rxe, ibdev_name);
> > > > >        rxe_set_mtu(rxe, mtu);
> > > > >
> > > > > -    return rxe_register_device(rxe, ibdev_name);
> > > > > +    return ret;
> > > > >     }
> > > > >
> > > > >     static int rxe_newlink(const char *ibdev_name, struct net_device *ndev)
> > > > > --
> > > > > 2.29.2
> > > > >