[dpdk-dev,2/6] net/sfc: add support for driver-wide dynamic logging
Checks
Commit Message
From: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Reviewed-by: Andy Moreton <amoreton@solarflare.com>
---
doc/guides/nics/sfc_efx.rst | 19 +++++++++++++++++++
drivers/net/sfc/efsys.h | 3 ++-
drivers/net/sfc/sfc_dp.c | 5 +++--
drivers/net/sfc/sfc_dp.h | 6 ++++--
drivers/net/sfc/sfc_ef10_rx.c | 4 ++--
drivers/net/sfc/sfc_ethdev.c | 13 +++++++++++++
drivers/net/sfc/sfc_log.h | 12 ++++++++++++
7 files changed, 55 insertions(+), 7 deletions(-)
Comments
On Thu, 25 Jan 2018 17:00:43 +0000
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
> index c7a54c3..8dd225e 100644
> --- a/drivers/net/sfc/efsys.h
> +++ b/drivers/net/sfc/efsys.h
> @@ -26,6 +26,7 @@
> #include <rte_io.h>
>
> #include "sfc_debug.h"
> +#include "sfc_log.h"
>
> #ifdef __cplusplus
> extern "C" {
> @@ -721,7 +722,7 @@ typedef uint64_t efsys_stat_t;
> #define EFSYS_ERR(_esip, _code, _dword0, _dword1) \
> do { \
> (void)(_esip); \
> - RTE_LOG(ERR, PMD, "FATAL ERROR #%u (0x%08x%08x)\n", \
> + SFC_GENERIC_LOG(ERR, "FATAL ERROR #%u (0x%08x%08x)", \
> (_code), (_dword0), (_dword1)); \
> _NOTE(CONSTANTCONDITION); \
> } while (B_FALSE)
Off topic, but why is this header file having C++ wrapper?
It is driver private, and driver is always built with C.
On 01/25/2018 09:42 PM, Stephen Hemminger wrote:
> On Thu, 25 Jan 2018 17:00:43 +0000
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> diff --git a/drivers/net/sfc/efsys.h b/drivers/net/sfc/efsys.h
>> index c7a54c3..8dd225e 100644
>> --- a/drivers/net/sfc/efsys.h
>> +++ b/drivers/net/sfc/efsys.h
>> @@ -26,6 +26,7 @@
>> #include <rte_io.h>
>>
>> #include "sfc_debug.h"
>> +#include "sfc_log.h"
>>
>> #ifdef __cplusplus
>> extern "C" {
>> @@ -721,7 +722,7 @@ typedef uint64_t efsys_stat_t;
>> #define EFSYS_ERR(_esip, _code, _dword0, _dword1) \
>> do { \
>> (void)(_esip); \
>> - RTE_LOG(ERR, PMD, "FATAL ERROR #%u (0x%08x%08x)\n", \
>> + SFC_GENERIC_LOG(ERR, "FATAL ERROR #%u (0x%08x%08x)", \
>> (_code), (_dword0), (_dword1)); \
>> _NOTE(CONSTANTCONDITION); \
>> } while (B_FALSE)
> Off topic, but why is this header file having C++ wrapper?
> It is driver private, and driver is always built with C.
In this particular case it is just few lines which, as I understand,
never hurt. So, it is better to have and not think about it.
I have no strong opinion on it.
On 1/25/2018 5:00 PM, Andrew Rybchenko wrote:
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
<...>
> @@ -2082,3 +2084,14 @@ RTE_PMD_REGISTER_PARAM_STRING(net_sfc_efx,
> SFC_KVARG_STATS_UPDATE_PERIOD_MS "=<long> "
> SFC_KVARG_MCDI_LOGGING "=" SFC_KVARG_VALUES_BOOL " "
> SFC_KVARG_DEBUG_INIT "=" SFC_KVARG_VALUES_BOOL);
> +
> +RTE_INIT(sfc_driver_register_logtype);
> +static void
> +sfc_driver_register_logtype(void)
> +{
> + int ret;
> +
> + ret = rte_log_register_type_and_pick_level(SFC_LOGTYPE_PREFIX "driver",
> + RTE_LOG_NOTICE);
No benefit of using rte_log_register_type_and_pick_level() here, in this stage
"opt_loglevel_list" will be empty and this will be same as rte_log_register()
On 03/05/2018 05:59 PM, Ferruh Yigit wrote:
> On 1/25/2018 5:00 PM, Andrew Rybchenko wrote:
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
> <...>
>
>> @@ -2082,3 +2084,14 @@ RTE_PMD_REGISTER_PARAM_STRING(net_sfc_efx,
>> SFC_KVARG_STATS_UPDATE_PERIOD_MS "=<long> "
>> SFC_KVARG_MCDI_LOGGING "=" SFC_KVARG_VALUES_BOOL " "
>> SFC_KVARG_DEBUG_INIT "=" SFC_KVARG_VALUES_BOOL);
>> +
>> +RTE_INIT(sfc_driver_register_logtype);
>> +static void
>> +sfc_driver_register_logtype(void)
>> +{
>> + int ret;
>> +
>> + ret = rte_log_register_type_and_pick_level(SFC_LOGTYPE_PREFIX "driver",
>> + RTE_LOG_NOTICE);
> No benefit of using rte_log_register_type_and_pick_level() here, in this stage
> "opt_loglevel_list" will be empty and this will be same as rte_log_register()
That's true except "uniform approach is good". I.e. simply use
rte_log_register_type_and_pick_level() everywhere to make it safe against
code movements.
In fact it was raised during internal review and we kept as you can see it.
Other option is to avoid usage of constructor here at all and move it to
probe.
Yes, it will be tried many times, but there is no harm if it is already
registered.
On 03/06/2018 05:45 PM, Andrew Rybchenko wrote:
> On 03/05/2018 05:59 PM, Ferruh Yigit wrote:
>> On 1/25/2018 5:00 PM, Andrew Rybchenko wrote:
>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>
>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> <...>
>>
>>> @@ -2082,3 +2084,14 @@ RTE_PMD_REGISTER_PARAM_STRING(net_sfc_efx,
>>> SFC_KVARG_STATS_UPDATE_PERIOD_MS "=<long> "
>>> SFC_KVARG_MCDI_LOGGING "=" SFC_KVARG_VALUES_BOOL " "
>>> SFC_KVARG_DEBUG_INIT "=" SFC_KVARG_VALUES_BOOL);
>>> +
>>> +RTE_INIT(sfc_driver_register_logtype);
>>> +static void
>>> +sfc_driver_register_logtype(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = rte_log_register_type_and_pick_level(SFC_LOGTYPE_PREFIX
>>> "driver",
>>> + RTE_LOG_NOTICE);
>> No benefit of using rte_log_register_type_and_pick_level() here, in
>> this stage
>> "opt_loglevel_list" will be empty and this will be same as
>> rte_log_register()
>
> That's true except "uniform approach is good". I.e. simply use
> rte_log_register_type_and_pick_level() everywhere to make it safe against
> code movements.
> In fact it was raised during internal review and we kept as you can
> see it.
>
> Other option is to avoid usage of constructor here at all and move it
> to probe.
> Yes, it will be tried many times, but there is no harm if it is
> already registered.
In fact it could be really required if dynamic library is used and it is
pulled later using dlopen() - don't know if there are any restrictions in
DPDK which prevent it.
On 3/6/2018 2:56 PM, Andrew Rybchenko wrote:
>
> In fact it could be really required if dynamic library is used and it is
> pulled later using dlopen() - don't know if there are any restrictions in
> DPDK which prevent it.
That function has constructor attribute, not sure how it works for that case.
I am good as long as this is not missed but decided to implement this way.
@@ -305,3 +305,22 @@ boolean parameters value.
only possible to set an arbitrary value on SFN8xxx provided that
firmware version is 6.2.1.1033 or higher, otherwise any positive
value will select a fixed update period of **1000** milliseconds
+
+
+Dynamic Logging Parameters
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+One may leverage EAL option "--log-level" to change default levels
+for the log types supported by the driver. The option is used with
+an argument typically consisting of two parts separated by a comma.
+
+Level value is the last part which takes an integer greater than 0.
+Log type is the former part which may contain a regular expression.
+Depending on the choice of the expression, the given log level may
+be used either for some specific log type or for a subset of types.
+
+SFC EFX PMD provides the following log types available for control:
+
+- ``pmd.net.sfc.driver`` (default level is **6** - ``RTE_LOG_NOTICE``)
+
+ Affects driver-wide messages unrelated to any particular devices.
@@ -26,6 +26,7 @@
#include <rte_io.h>
#include "sfc_debug.h"
+#include "sfc_log.h"
#ifdef __cplusplus
extern "C" {
@@ -721,7 +722,7 @@ typedef uint64_t efsys_stat_t;
#define EFSYS_ERR(_esip, _code, _dword0, _dword1) \
do { \
(void)(_esip); \
- RTE_LOG(ERR, PMD, "FATAL ERROR #%u (0x%08x%08x)\n", \
+ SFC_GENERIC_LOG(ERR, "FATAL ERROR #%u (0x%08x%08x)", \
(_code), (_dword0), (_dword1)); \
_NOTE(CONSTANTCONDITION); \
} while (B_FALSE)
@@ -14,6 +14,7 @@
#include <rte_log.h>
#include "sfc_dp.h"
+#include "sfc_log.h"
void
sfc_dp_queue_init(struct sfc_dp_queue *dpq, uint16_t port_id, uint16_t queue_id,
@@ -63,8 +64,8 @@ int
sfc_dp_register(struct sfc_dp_list *head, struct sfc_dp *entry)
{
if (sfc_dp_find_by_name(head, entry->type, entry->name) != NULL) {
- rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD,
- "sfc %s dapapath '%s' already registered\n",
+ SFC_GENERIC_LOG(ERR,
+ "sfc %s dapapath '%s' already registered",
entry->type == SFC_DP_RX ? "Rx" :
entry->type == SFC_DP_TX ? "Tx" :
"unknown",
@@ -15,6 +15,8 @@
#include <rte_pci.h>
+#include "sfc_log.h"
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -58,10 +60,10 @@ void sfc_dp_queue_init(struct sfc_dp_queue *dpq,
const struct sfc_dp_queue *_dpq = (dpq); \
const struct rte_pci_addr *_addr = &(_dpq)->pci_addr; \
\
- RTE_LOG(level, PMD, \
+ SFC_GENERIC_LOG(level, \
RTE_FMT("%s " PCI_PRI_FMT \
" #%" PRIu16 ".%" PRIu16 ": " \
- RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+ RTE_FMT_HEAD(__VA_ARGS__ ,), \
dp_name, \
_addr->domain, _addr->bus, \
_addr->devid, _addr->function, \
@@ -587,8 +587,8 @@ sfc_ef10_supported_ptypes_get(uint32_t tunnel_encaps)
1u << EFX_TUNNEL_PROTOCOL_NVGRE):
return ef10_overlay_ptypes;
default:
- RTE_LOG(ERR, PMD,
- "Unexpected set of supported tunnel encapsulations: %#x\n",
+ SFC_GENERIC_LOG(ERR,
+ "Unexpected set of supported tunnel encapsulations: %#x",
tunnel_encaps);
/* FALLTHROUGH */
case 0:
@@ -27,6 +27,8 @@
#include "sfc_dp.h"
#include "sfc_dp_rx.h"
+uint32_t sfc_logtype_driver;
+
static struct sfc_dp_list sfc_dp_head =
TAILQ_HEAD_INITIALIZER(sfc_dp_head);
@@ -2082,3 +2084,14 @@ RTE_PMD_REGISTER_PARAM_STRING(net_sfc_efx,
SFC_KVARG_STATS_UPDATE_PERIOD_MS "=<long> "
SFC_KVARG_MCDI_LOGGING "=" SFC_KVARG_VALUES_BOOL " "
SFC_KVARG_DEBUG_INIT "=" SFC_KVARG_VALUES_BOOL);
+
+RTE_INIT(sfc_driver_register_logtype);
+static void
+sfc_driver_register_logtype(void)
+{
+ int ret;
+
+ ret = rte_log_register_type_and_pick_level(SFC_LOGTYPE_PREFIX "driver",
+ RTE_LOG_NOTICE);
+ sfc_logtype_driver = (ret < 0) ? RTE_LOGTYPE_PMD : ret;
+}
@@ -10,6 +10,18 @@
#ifndef _SFC_LOG_H_
#define _SFC_LOG_H_
+/** Generic driver log type */
+extern uint32_t sfc_logtype_driver;
+
+/** Common log type name prefix */
+#define SFC_LOGTYPE_PREFIX "pmd.net.sfc."
+
+/** Log PMD generic message, add a prefix and a line break */
+#define SFC_GENERIC_LOG(level, ...) \
+ rte_log(RTE_LOG_ ## level, sfc_logtype_driver, \
+ RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
+ RTE_FMT_TAIL(__VA_ARGS__ ,)))
+
/* Log PMD message, automatically add prefix and \n */
#define SFC_LOG(sa, level, ...) \
do { \