On Tue, Jan 30, 2024 at 08:13:21PM +0000, Dexuan Cui wrote:
> > From: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx>
> > Sent: Monday, January 29, 2024 11:19 PM
> > [...]
> > If hv_netvsc driver is removed and reloaded, the NET_DEVICE_REGISTER
>
> s/removed/unloaded/
> unloaded looks more accurate to me :-)
>
> > [...]
> > Tested-on: Ubuntu22
> > Testcases: LISA testsuites
> > verify_reload_hyperv_modules, perf_tcp_ntttcp_sriov
> IMO the 3 lines can be removed: this bug is not specific to Ubuntu, and the
> test case names don't provide extra value to help understand the issue
> here and they might cause more questions unnecessarily, e.g. what's LISA,
> what exactly do the test cases do.
>
> > +/* Macros to define the context of vf registration */
> > +#define VF_REG_IN_PROBE 1
> > +#define VF_REG_IN_RECV_CBACK 2
>
> I think VF_REG_IN_NOTIFIER is a better name?
> RECV_CBALL looks inaccurate to me.
>
> > @@ -2205,8 +2209,11 @@ static int netvsc_vf_join(struct net_device
> > *vf_netdev,
> > ndev->name, ret);
> > goto upper_link_failed;
> > }
> > -
> > - schedule_delayed_work(&ndev_ctx->vf_takeover,
> > VF_TAKEOVER_INT);
> > + /* If this registration is called from probe context vf_takeover
> > + * is taken care of later in probe itself.
> I suspect "later in probe itself" is not accurate.
> If 'context' is VF_REG_IN_PROBE, I suppose what happens here is:
> after netvsc_probe() finishes, the netvsc interface becomes available,
> so the user space will ifup it, and netvsc_open() will UP the VF interface,
> and netvsc_netdev_event() is called for the VF with event ==
> NETDEV_POST_INIT (?) and NETDEV_CHANGE, and the data path is
> switched to the VF.
>
> If my understanding is correct, I think in the case of 'context' ==
> VF_REG_IN_PROBE, I suspect the "Align MTU of VF with master"
> and the "sync address list from ndev to VF" in __netvsc_vf_setup() are
> omitted? If so, should this be fixed? e.g. Not sure if the below is an issue or not:
> 1) a VF is bound to a NetVSC interface, and a user sets the MTUs to 1024.
> 2) rmmod hv_netvsc
> 3) modprobe hv_netvsc
> 4) the netvsc interface uses MTU=1500 (the default), and the VF still uses 1024.
>
> > @@ -2597,6 +2604,34 @@ static int netvsc_probe(struct hv_device *dev,
> > }
> >
> > list_add(&net_device_ctx->list, &netvsc_dev_list);
> > +
> > + /* When the hv_netvsc driver is removed and readded, the
>
> s/removed and readded/unloaded and reloaded/
>
> > + * NET_DEVICE_REGISTER for the vf device is replayed before
> > probe
> > + * is complete. This is because register_netdevice_notifier() gets
> > + * registered before vmbus_driver_register() so that callback func
> > + * is set before probe and we don't miss events like
> > NETDEV_POST_INIT
> > + * So, in this section we try to register each matching
>
> Looks like there are spaces at the end of the line. We can move up a few words
> from the next line :-)
>
> s/each matching/the matching/
> A NetVSC interface has only 1 matching VF device.
>
> > + * vf device that is present as a netdevice, knowing that it's register
>
> s/it's/its/
>
> > + * call is not processed in the netvsc_netdev_notifier(as probing is
> > + * progress and get_netvsc_byslot fails).
> > + */
> > + for_each_netdev(dev_net(net), vf_netdev) {
> > + if (vf_netdev->netdev_ops == &device_ops)
> > + continue;
> > +
> > + if (vf_netdev->type != ARPHRD_ETHER)
> > + continue;
> > +
> > + if (is_vlan_dev(vf_netdev))
> > + continue;
> > +
> > + if (netif_is_bond_master(vf_netdev))
> > + continue;
>
> The code above is duplicated from netvsc_netdev_event().
> Can we add a new helper function is_matching_vf() to avoid the duplication?
Sure, I will do that. Thanks
>
> > + netvsc_prepare_bonding(vf_netdev);
> > + netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);
> > + __netvsc_vf_setup(net, vf_netdev);
>
> add a "break;' ?
With MANA devices and multiport support there, the individual ports are also net_devices.
Wouldn't this be needed for such scenario(where we have multiple mana port net devices) to
register them all?
>
> > + }
> > rtnl_unlock();
