Hi Aaron,
First of all thank you very much for your feedback.
I didn't pay that attention to the style, I'll correct all the issues when
I send a proper patch, by the way, is there any script or tool to check the
style?, I looked for it and I cannot find anything.
On Wed, Mar 23, 2016 at 11:42 AM, Aaron Conole <acon...@redhat.com> wrote:
> Hi Mauricio,
>
> Mauricio Vasquez B <mauricio.vasquezber...@studenti.polito.it> writes:
>
> > In order to use dpdk ports in ovs they have to be bound to a DPDK
> > compatible driver before ovs is started.
> >
> > This patch adds the possibility to hotplug (or hot-unplug) a device
> > after ovs has been started. The implementation adds two appctl commands:
> > netdev-dpdk/attach and netdev-dpdk/detach.
> >
> > After the user attaches a new device, it can use the add-port command
> > to use it in a switch, similarly, before detaching a device, it has to
> > be removed using the del-port command.
> >
> > Signed-off-by: Mauricio Vasquez B <
> mauricio.vasquezber...@studenti.polito.it>
> > ---
>
> I like the patch a lot, it really helps with the ease-of-use of
> OVS. So, thanks very much for the work.
>
> > lib/netdev-dpdk.c | 79
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index f402354..c3a99e7 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2253,12 +2253,91 @@ dpdk_vhost_user_class_init(void)
> > }
> >
> > static void
> > +netdev_dpdk_attach(struct unixctl_conn *conn, int argc,
> > + const char *argv[], void *aux OVS_UNUSED)
> ^ I think there's a spacing issue here.
>
> > +{
> > + int ret;
> > + uint8_t port_id;
> > + char response[512];
> > +
> > + ovs_mutex_lock(&dpdk_mutex);
> > +
> > + ret = rte_eth_dev_attach(argv[1], &port_id);
> > + if(ret < 0) {
> ^ Spacing issue here
>
> > + snprintf(response, sizeof(response),
> > + "Error attaching device ''", argv[1]);
> ^ Spacing issue here
>
> > + unixctl_command_reply_error(conn, response);
> > + goto unlock;
> > + }
> > +
>
> Does it make sense to also create the netdev at this point?
>
Personally, I think that it is better not to create it, I have these two
reasons for the moment:
1. Creating a port does not only implies creating the netdev in
netdev-dpdk, it implies creating the port in all the upper layers. It would
be necessary to use ovsdb in order to do it, I don't know if it is allowed
to use ovsdb in that way from a netdev-provider.
2. If the user invokes some external element (an orchestrator for example)
that adds the port, after attaching the port, it could fail due to the port
is already there.
> > + snprintf(response, sizeof(response),
> > + "Device '%s' has been attached as 'dpdk%d'", argv[1],
> port_id);
>
> ^ Spacing issue here.
>
> > + unixctl_command_reply(conn, response);
> > +
> > +unlock:
> > + ovs_mutex_unlock(&dpdk_mutex);
> > +}
> > +
> > +static void
> > +netdev_dpdk_detach(struct unixctl_conn *conn, int argc,
> > + const char *argv[], void *aux OVS_UNUSED)
> ^ Spacing issue here.
>
> > +{
> > + int ret;
> > + unsigned int port_id;
> > + char name[RTE_ETH_NAME_MAX_LEN];
> > + char response[512];
> > +
> > + ret = dpdk_dev_parse_name(argv[1], "dpdk", &port_id);
> > + if(ret) {
> ^ Spacing issue here.
>
> > + snprintf(response, sizeof(response),
> > + "'%s' is not a valid dpdk device", argv[1]);
> ^ Spacing issue here.
>
> > + unixctl_command_reply_error(conn, response);
> > + return;
> > + }
> > +
> > + ovs_mutex_lock(&dpdk_mutex);
> > +
> > + struct netdev * netdev = netdev_from_name(argv[1]);
> > + if(netdev) {
> ^ Spacing issue here.
>
> > + netdev_close(netdev);
> > + snprintf(response, sizeof(response),
> > + "Port '%s' is being used. Remove it before
> detaching", argv[1]);
> > + unixctl_command_reply_error(conn, response);
> > + goto unlock;
> > + }
> > +
> > + rte_eth_dev_close(port_id);
> > +
> > + ret = rte_eth_dev_detach(port_id, name);
> > + if(ret < 0) {
> ^ Spacing issue here.
>
> > + snprintf(response, sizeof(response),
> > + "Port '%s' can not be detached", argv[1]);
> > + unixctl_command_reply_error(conn, response);
> > + goto unlock;
> > + }
> > +
> > + snprintf(response, sizeof(response), "Port '%s' has been detached",
> argv[1]);
> ^ Please stick to 80-bytes on a line.
>
> > + unixctl_command_reply(conn, response);
> > +
> > +unlock:
> > + ovs_mutex_unlock(&dpdk_mutex);
> > +}
> > +
> > +static void
> > dpdk_common_init(void)
> > {
> > unixctl_command_register("netdev-dpdk/set-admin-state",
> > "[netdev] up|down", 1, 2,
> > netdev_dpdk_set_admin_state, NULL);
> >
> > + unixctl_command_register("netdev-dpdk/attach",
> > + "device", 1, 1,
> > + netdev_dpdk_attach, NULL);
> > +
> > + unixctl_command_register("netdev-dpdk/detach",
> > + "port", 1, 1,
> > + netdev_dpdk_detach, NULL);
> > +
> Does it make sense to have a single command ``netdev-dpdk/port-ctl''
> that takes either attach/detach as an argument? I think it looks a bit
> better, and you might be able to combine a lot of the code above into a
> single function, but I have been wrong on user-experience stuff before.
>
Yes, it makes a lot of sense, my only concern is that both parameters
(attach and detach) would not receive the same type of argument, attach
receives the pci address while detach receives the port name, however I
think it is not a big problem, there are more advantages in creating a
single command.
I will send a proper patch updating also the documentation.
>
> > ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
> > }
>
> Thanks again,
> -Aaron
>
Mauricio V,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
