Answers inlined. > Subiect: Re: [ovs-dev] [PATCH] [RFC][windows]: Remap VPORT socket pool > > Hi Alin, > > Thanks for the patch. Shashank is giving it a run. How do we prevent this > issue in the future? [Alin Serdean] Thanks for trying it out, and I'm sorry I haven't caught this issue in time.
The best way would be to have a CI and run at least some sanity checks on the windows datapath when it is enabled. I can work on getting some tests from the: `make check-kernel` and `make check-kmod` to run on Windows and we can reuse most of the logic. Another useful CI would be to run the unit tests on Windows as well. > > For starters, I think we need to consolidate the usage under nl_* functions to > ensure it's not doing something differently in vport_*. [Alin Serdean] I agree. We should also spend some cycles switching to IOCPs IMO. > > We need to look into investing in the test framework for catching basic > Windows failures like these. [Alin Serdean] We can reuse the test framework that is used on Linux as I pointed above the biggest issue IMO is where and how we run it. > > Thanks, > Sairam > > On 11/12/18, 8:51 AM, "[email protected] on behalf of Alin > Gabriel Serdean" <[email protected] on behalf of > [email protected]> wrote: > > Fixes: > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub > .com%2Fopenvswitch%2Fovs- > issues%2Fissues%2F164&data=02%7C01%7Cvsairam%40vmware.com% > 7C76dd26cc93e145b3a12008d648bf0d08%7Cb39138ca3cee4b4aa4d6cd83d9dd > 62f0%7C1%7C0%7C636776382753818056&sdata=E%2FWSjXq%2BMfQls > GG9Fw59FblJxGTjOXYVFpNUKJtyp0k%3D&reserved=0 > > Signed-off-by: Alin Gabriel Serdean <[email protected]> > --- > lib/dpif-netlink.c | 58 > +++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 69c145cc3..5994c3445 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -246,6 +246,38 @@ static int dpif_netlink_port_query__(const struct > dpif_netlink *dpif, > odp_port_t port_no, const char > *port_name, > struct dpif_port *dpif_port); > > + > +static struct nl_sock * > +vport_create_socksp_windows(struct dpif_netlink *dpif, int *error) > +OVS_REQ_WRLOCK(dpif->upcall_lock) > +{ > + struct nl_sock *socksp; > + size_t i; > + /* Pick netlink sockets to use in a round-robin fashion from each > + * handler's pool of sockets. */ > + struct dpif_handler *handler = &dpif->handlers[0]; > + struct dpif_windows_vport_sock *sock_pool = handler- > >vport_sock_pool; > + size_t index = handler->last_used_pool_idx; > + /* A pool of sockets is allocated when the handler is initialized. */ > + if (sock_pool == NULL) { > + *error = EINVAL; > + return NULL; > + } > + ovs_assert(index < VPORT_SOCK_POOL_SIZE); > + socksp = sock_pool[index].nl_sock; > + ovs_assert(socksp); > + index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1; > + handler->last_used_pool_idx = index; > + *error = 0; > + return socksp; > +} > +static void > +vport_del_socksp_windows(struct dpif_netlink *dpif, struct nl_sock > *socksp) > +{ > + socksp = NULL; > +} > + > + > static struct dpif_netlink * > dpif_netlink_cast(const struct dpif *dpif) > { > @@ -716,7 +748,12 @@ dpif_netlink_port_add__(struct dpif_netlink > *dpif, const char *name, > int error = 0; > > if (dpif->handlers) { > - if (nl_sock_create(NETLINK_GENERIC, &socksp)) { > +#ifdef _WIN32 > + socksp = vport_create_socksp_windows(dpif, &error); > +#else > + error = nl_sock_create(NETLINK_GENERIC, &socksp); > +#endif > + if (!socksp) { > return error; > } > } > @@ -748,7 +785,11 @@ dpif_netlink_port_add__(struct dpif_netlink > *dpif, const char *name, > dpif_name(&dpif->dpif), *port_nop); > } > > +#ifdef _WIN32 > + vport_del_socksp_windows(dpif, socksp); > +#else > nl_sock_destroy(socksp); > +#endif > goto exit; > } > > @@ -763,7 +804,11 @@ dpif_netlink_port_add__(struct dpif_netlink > *dpif, const char *name, > request.dp_ifindex = dpif->dp_ifindex; > request.port_no = *port_nop; > dpif_netlink_vport_transact(&request, NULL, NULL); > +#ifdef _WIN32 > + vport_del_socksp_windows(dpif, socksp); > +#else > nl_sock_destroy(socksp); > +#endif > goto exit; > } > > @@ -2249,15 +2294,26 @@ dpif_netlink_refresh_channels(struct > dpif_netlink *dpif, uint32_t n_handlers) > || !vport_get_pid(dpif, port_no, &upcall_pid)) { > struct nl_sock *socksp; > > +#ifdef _WIN32 > + socksp = vport_create_socksp_windows(dpif, &error); > + if (!socksp) { > + goto error; > + } > +#else > if (nl_sock_create(NETLINK_GENERIC, &socksp)) { > goto error; > } > +#endif > > error = vport_add_channel(dpif, vport.port_no, socksp); > if (error) { > VLOG_INFO("%s: could not add channels for port %s", > dpif_name(&dpif->dpif), vport.name); > +#ifdef _WIN32 > + vport_del_socksp_windows(dpif, socksp); > +#else > nl_sock_destroy(socksp); > +#endif > retval = error; > goto error; > } > -- > 2.16.1.windows.1 > > _______________________________________________ > dev mailing list > [email protected] > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.o > penvswitch.org%2Fmailman%2Flistinfo%2Fovs- > dev&data=02%7C01%7Cvsairam%40vmware.com%7C76dd26cc93e145b > 3a12008d648bf0d08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C > 636776382753818056&sdata=7ywMj5jsv3iLcXrTuDlqIvvZcojQtmlwVDjDs > AGi3Es%3D&reserved=0 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
