I sent a proper patch:
http://openvswitch.org/pipermail/dev/2016-March/068600.html
On Thu, Mar 24, 2016 at 11:57 AM, Mauricio Vásquez <
mauricio.vasquezber...@studenti.polito.it> wrote:
> 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
