| Message ID | 20240413084826.52417-1-jun.gu@easystack.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: openvswitch: Check vport name | expand |
On 13 Apr 2024, at 10:48, jun.gu wrote: > Check vport name from dev_get_by_name, this can avoid to add and remove > NIC repeatedly when NIC rename failed at system startup. > > Signed-off-by: Jun Gu <jun.gu@easystack.cn> > --- > net/openvswitch/vport-netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c > index 903537a5da22..de8977d7f329 100644 > --- a/net/openvswitch/vport-netdev.c > +++ b/net/openvswitch/vport-netdev.c > @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) > int err; > > vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); > - if (!vport->dev) { > + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name. So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something? > err = -ENODEV; > goto error_free_vport; > } > -- > 2.25.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 16 Apr 2024, at 9:57, Jun Gu wrote: > Hi Chaudron, thanks for your reply. Considerthe following scenario: > > - set a net device alias name (such as *enpxx*) into ovs. > > - |OVS_VPORT_CMD_NEW -> ||dev_get_by_name can query the net device using *enpxx* name, but the dev->name that return is *ensxx* name. the |net_device|struct including: ``` |struct net_device { > char name[IFNAMSIZ]; > RH_KABI_REPLACE(struct hlist_node name_hlist, > struct netdev_name_node *name_node) > ... > |``` name_hlist include enpxx and ensxx name and both of them point to the same net_device, the net_device's name is ensxx. So when using *enpxx* name to query net device, the ||net device's name that return is *ensxx* name.| > Then, openvswitch.ko will save the net device that name is*ensxx* to a hash table. So this will cause the below process: > - ovs can'tobtain the enpxx net device by |OVS_VPORT_CMD_GET.| > -|dpif_netlink_port_poll will get a |notification after |OVS_VPORT_CMD_NEW|, but the vport's name is ensxx. ovs will query the ensxx net device from interface table, but nothing. So ovs will run the port_del operation. > - this will cause|OVS_VPORT_CMD_NEW| and OVS_VPORT_CMD_DEL operation to run repeatedly. > So the above issue can be avoided by the patch. Thanks for the clarification, I forgot about the alias case. And you are right, OVS userspace does not support using interface aliases. Maybe you can update the commit message, and add a code comment to clarify this in your next revision? Cheers, Eelco > 在 4/15/24 18:04, Eelco Chaudron 写道: >> >> On 13 Apr 2024, at 10:48, jun.gu wrote: >> >>> Check vport name from dev_get_by_name, this can avoid to add and remove >>> NIC repeatedly when NIC rename failed at system startup. >>> >>> Signed-off-by: Jun Gu<jun.gu@easystack.cn> >>> --- >>> net/openvswitch/vport-netdev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c >>> index 903537a5da22..de8977d7f329 100644 >>> --- a/net/openvswitch/vport-netdev.c >>> +++ b/net/openvswitch/vport-netdev.c >>> @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) >>> int err; >>> >>> vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); >>> - if (!vport->dev) { >>> + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { >> Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name. >> >> So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something? >> >>> err = -ENODEV; >>> goto error_free_vport; >>> } >>> -- >>> 2.25.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Sat, 2024-04-13 at 16:48 +0800, jun.gu wrote: > Check vport name from dev_get_by_name, this can avoid to add and remove > NIC repeatedly when NIC rename failed at system startup. > > Signed-off-by: Jun Gu <jun.gu@easystack.cn> > --- > net/openvswitch/vport-netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c > index 903537a5da22..de8977d7f329 100644 > --- a/net/openvswitch/vport-netdev.c > +++ b/net/openvswitch/vport-netdev.c > @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) > int err; > > vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); > - if (!vport->dev) { > + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { ^^ Missing open bracket here, and close bracket towards the end. Please build and test your patch before submitting the next revision. Please also include the target tree ('net-next') in the subj prefix when you will submit it. And update the commit message as per Eelco's request. Thanks, Paolo
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c index 903537a5da22..de8977d7f329 100644 --- a/net/openvswitch/vport-netdev.c +++ b/net/openvswitch/vport-netdev.c @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name) int err; vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name); - if (!vport->dev) { + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { err = -ENODEV; goto error_free_vport; }
Check vport name from dev_get_by_name, this can avoid to add and remove NIC repeatedly when NIC rename failed at system startup. Signed-off-by: Jun Gu <jun.gu@easystack.cn> --- net/openvswitch/vport-netdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)