>>
>>    Compute Node            Controller            Compute Node
>>
>>     Collectd  <----------> Ceilometer <-------->   Collectd
>>
>>     OvS DPDK                                       OvS DPDK
>>
>>    +-----+
>>    | VM  |
>>    +--+--+
>>   \---+---/
>>       |
>>    +--+---+       +------------+----------+     +------+-------+
>>    | OVS  |-----> |    dpdkevents plugin  | --> |   collectd   |
>>    +--+---+       +------------+----------+     +------+-------+
>>                                                        |
>>  +------+-----+     +---------------+------------+     |
>>  | Ceilometer | <-- | collectd ceilometer plugin |  <---
>>  +------+-----+     +---------------+------------+
>
>You see all of this, right here ^^^ ? That's excellent. Put *that* in a doc. I
>would suggest 'Documentation/topics/dpdk/keepalive.rst'. You could include
>a reference to the below doc using something like the
>following:
>
>    For information on how to use the keepalive feature, refer to
>    :ref:`the HOWTO <dpdk_keepalive>`.

I have plans to write a separate document with detail explanation on how this 
feature should be used with OpenStack. That also includes how this is 
integrated with ceilometer. But I will do the document as a separate patch once 
this feature gets accepted. 

>
>The only changes I'd make is to indent the diagram by four spaces and
>precede it with by '::' (to format as a literal block), and change the OVSDB
>settings overview piece to use definitions lists, which look like
>this:
>
>    ``keepalive=true``
>
>      Enable the keepalive feature. Defaults to false (disabled).
>
>This could be done as a separate patch unless you need to respin this series.

I will take this suggestion and do this if I have to send v3.

>
>> Performance impact:
>> No noticeable performance or latency impact is observed with KA
>> feature enabled. The tests were run with 100ms KA interval and latency
>> is (Min:134,710ns, Avg:173,005ns, Max:1,504,670ns) for Phy2Phy
>> loopback test case with 100 unique streams.
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> <bhanuprakash.bodire...@intel.com>
>> ---
>>  Documentation/howto/dpdk.rst | 93
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>
>> diff --git a/Documentation/howto/dpdk.rst
>> b/Documentation/howto/dpdk.rst index dc63f7d..e482166 100644
>> --- a/Documentation/howto/dpdk.rst
>> +++ b/Documentation/howto/dpdk.rst
>> @@ -400,6 +400,99 @@ If ``N`` is set to 1, an insertion will be
>> performed for every flow. If set to
>>
>>  For more information on the EMC refer to :doc:`/intro/install/dpdk` .
>>
>> +.. _dpdk_keepalive:
>> +
>> +KeepAlive
>> +---------
>> +
>> +OvS KeepAlive(KA) feature is disabled by default. To enable KA
>> feature::
>
>s/OvS/OVS/
>s/KeepAlive(KA)/KeepAlive (KA)/
>
Ok.

>> +
>> +    $ ovs-vsctl --no-wait set Open_vSwitch .
>> other_config:keepalive=true
>> +
>> +The default timer interval for monitoring packet processing cores is
>> 100ms.
>> +To set a different timer value, run::
>> +
>> +    $ ovs-vsctl --no-wait set Open_vSwitch . \
>> +        other_config:keepalive-interval="50"
>> +
>> +The events comprise of core states and the last seen timestamps. The
>> events
>> +are written in to shared memory region
>> ``/dev/shm/dpdk_keepalive_shm_name``.
>> +To write in to a different shared memory region, run::
>> +
>> +    $ ovs-vsctl --no-wait set Open_vSwitch . \
>> +        other_config:keepalive-shm-name="/<shared memory region>"
>> +
>
>nit: I assume the '/' before '<shared memory region>' was a typo. Drop that, if
>so.
It's not a typo. It is expected.

>
>> +The events in the shared memory block can be read by external
>> monitoring
>> +framework (or) applications. `collectd <https://collectd.org/>`__
>> has built-in
>> +support for DPDK and provides a `dpdkevents` plugin that can be
>> enabled to
>> +relay the datapath core status to OpenStack service `Ceilometer
>> +<https://docs.openstack.org/developer/ceilometer/>`__.
>> +
>> +To install and configure `collectd`, run::
>> +
>> +    # Clone collectd from Git repository
>> +    $ git clone https://github.com/collectd/collectd.git
>> +
>> +    # configure and install collectd
>> +    $ cd collectd
>> +    $ ./build.sh
>> +    $ ./configure --enable-syslog --enable-logfile --with-
>> libdpdk=/usr
>> +    $ make
>> +    $ make install
>> +
>
>I should have called this out first time, but I'm not sure we want to duplicate
>collectd's installation procedure as these things can change.
>We might be better of linking to installation docs. _However_, we do this
>already for DPDK so there is precedent. If you think we should keep them
>(and you likely do), I'd be happy to simply include a link to the installation 
>docs
>like so:

While I agree to this, collectd documentation is vast and users may be lost 
reading the installation.
I would mention the basic steps here and can point a link to the documentation 
as suggested so that they can refer to the documentation for any advanced 
debugging.

>
>    For further information on installing and configuring collectd,
>    refer to the `collectd installation guide <???>`__.
>
>> +`collectd` is installed in ``/opt/collectd`` by default. Edit the
>> configuration
>> +file in ``/opt/collectd/etc/collectd.conf`` to enable logfile,
>> dpdkevents
>> +and csv plugin::
>
>nit:
>
>    ...to enable the `logfile`, `dpdkevents`, and `csv` plugins. To
>    enable the `logfile` plugin, add::
Ok.

>
>> +
>> +   LoadPlugin logfile
>> +   <Plugin logfile>
>> +       LogLevel debug
>> +       File "/var/log/collectd/collectd.log"
>> +       Timestamp true
>> +       PrintSeverity false
>> +   </Plugin>
>> +
>> +   <Plugin syslog>
>> +       LogLevel info
>> +   </Plugin>
>> +
>> +Enable `dpdkevents` plugin and update the plugindetails as below::
>
>Is 'plugindetails' a variable or file name, or simply a term you're using to
>reference the below chunk of XML? If the latter, perhaps something like:
>
>    To enable the `dpdkevents` plugin, add:

Ok, I was referring to the chunk of XML. 
>
>> +
>> +   LoadPlugin dpdkevents
>> +
>> +   <Plugin "dpdkevents">
>> +     <EAL>
>> +       Coremask "0x2"
>> +       MemoryChannels "4"
>> +       ProcessType "secondary"
>> +       FilePrefix "rte"
>> +     </EAL>
>> +     <Event "keep_alive">
>> +       SendEventsOnUpdate true
>> +       LCoreMask "0xf"
>> +       KeepAliveShmName "/dpdk_keepalive_shm_name"
>> +       SendNotification True
>> +      </Event>
>> +   </Plugin>
>> +
>> +``LCoreMask`` should be set to the PMD cores that were earlier
>> registered
>> +for keepalive monitoring. ``KeepAliveShmName`` refers to shared
>> memory block
>> +region.
>> +
>+Enable ``csv`` plugin as below::
>> +
>> +   LoadPlugin csv
>> +
>> +   <Plugin csv>
>> +       DataDir "/var/log/collectd/csv"
>> +       StoreRates false
>> +   </Plugin>
>> +
>> +With csv plugin enabled, *meter* or *gauge* file is created and
>> timestamp
>
>s/*meter*/a *meter*/
>
>> +and core status gets updated which are sent to ceilometer service.
>> For example
>> +``../csv/localhost/dpdkevents-keepalive/gauge-lcore3-2017-04-01`` is
>> the file
>> +for pmd thread running on core 3.
>> +
>>  .. _dpdk-ovs-in-guest:
>>
>>  OVS with DPDK Inside VMs
>
>All the above could be fixed at merge time or after merge, and I'd be happy to
>see this go in even as is. As such:
>
>Acked-by: Stephen Finucane <step...@that.guru>

Thanks Stephen for the review. Appreciate all your feedback.

- Bhanuprakash
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to