Re: [PATCH 1/5] ethdev: fix race-condition of proactive error handling mode
03/03/2023 16:51, Ferruh Yigit пишет:
On 3/2/2023 12:08 PM, Konstantin Ananyev wrote:
In the proactive error handling mode, the PMD will set the data path
pointers to dummy functions and then try recovery, in this period the
application may still invoking data path API. This will introduce a
race-condition with data path which may lead to crash [1].
Although the PMD added delay after setting data path pointers to cover
the above race-condition, it reduces the probability, but it doesn't
solve the problem.
To solve the race-condition problem fundamentally, the following
requirements are added:
1. The PMD should set the data path pointers to dummy functions after
report RTE_ETH_EVENT_ERR_RECOVERING event.
2. The application should stop data path API invocation when process
the RTE_ETH_EVENT_ERR_RECOVERING event.
3. The PMD should set the data path pointers to valid functions before
report RTE_ETH_EVENT_RECOVERY_SUCCESS event.
4. The application should enable data path API invocation when process
the RTE_ETH_EVENT_RECOVERY_SUCCESS event.
How this is solving the race-condition, by pushing responsibility to
stop data path to application?
Exactly, it becomes application responsibility to make sure data-path is
stopped/suspended before recovery will continue.
What if application is not interested in recovery modes at all and not
registered any callback for the recovery?
Are you saying there is no way for application to disable
automatic recovery in PMD if it is not interested
(or can't full-fill per-requesties for it)?
If so, then yes it is a problem and we need to fix it.
I assumed that such mechanism to disable unwanted events already exists,
but I can't find anything.
Wonder what would be the easiest way here - can PMD make a decision
based on callback return value, or do we need a new API to
enable/disable callbacks, or ...?
I think driver should not rely on application for this, unless
application explicitly says (to driver) that it is handling recovery,
right now there is no way for driver to know this.
I think it is visa-versa:
application should not enable auto-recovery if it can't meet
per-requeststies for it (provide appropriate callback).
Also, this patch introduce a driver internal function
rte_eth_fp_ops_setup which used as an help function for PMD.
[1]
http://patchwork.dpdk.org/project/dpdk/patch/20230220060839.1267349-2-ashok.k.kal...@intel.com/
Fixes: eb0d471a8941 ("ethdev: add proactive error handling mode")
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
---
doc/guides/prog_guide/poll_mode_drv.rst | 20 +++-
lib/ethdev/ethdev_driver.c | 8 +++
lib/ethdev/ethdev_driver.h | 10
lib/ethdev/rte_ethdev.h | 32 +++--
lib/ethdev/version.map | 1 +
5 files changed, 46 insertions(+), 25 deletions(-)
diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..e380ff135a 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -638,14 +638,9 @@ different from the application invokes recovery in PASSIVE
mode,
the PMD automatically recovers from error in PROACTIVE mode,
and only a small amount of work is required for the application.
-During error detection and automatic recovery,
-the PMD sets the data path pointers to dummy functions
-(which will prevent the crash),
-and also make sure the control path operations fail with a return code
``-EBUSY``.
-
-Because the PMD recovers automatically,
-the application can only sense that the data flow is disconnected for a while
-and the control API returns an error in this period.
+During error detection and automatic recovery, the PMD sets the data path
+pointers to dummy functions and also make sure the control path operations
+failed with a return code ``-EBUSY``.
In order to sense the error happening/recovering,
as well as to restore some additional configuration,
@@ -653,9 +648,9 @@ three events are available:
``RTE_ETH_EVENT_ERR_RECOVERING``
Notify the application that an error is detected
- and the recovery is being started.
+ and the recovery is about to start.
Upon receiving the event, the application should not invoke
- any control path function until receiving
+ any control and data path API until receiving
``RTE_ETH_EVENT_RECOVERY_SUCCESS`` or ``RTE_ETH_EVENT_RECOVERY_FAILED``
event.
.. note::
@@ -666,8 +661,9 @@ three events are available:
``RTE_ETH_EVENT_RECOVERY_SUCCESS``
Notify the application that the recovery from error is successful,
- the PMD already re-configures the port,
- and the effect is the same as a restart operation.
+ the PMD already re-configures the port.
+ The application should restore some additional configuration, and then
+ enable data path API invocation.
``RTE_ETH_EVENT_RECOVERY_FAILED``
Notify the a
Re: [PATCH 1/5] ethdev: fix race-condition of proactive error handling mode
In the proactive error handling mode, the PMD will set the data path
pointers to dummy functions and then try recovery, in this period the
application may still invoking data path API. This will introduce a
race-condition with data path which may lead to crash [1].
Although the PMD added delay after setting data path pointers to
cover the above race-condition, it reduces the probability, but it
doesn't solve the problem.
To solve the race-condition problem fundamentally, the following
requirements are added:
1. The PMD should set the data path pointers to dummy functions after
report RTE_ETH_EVENT_ERR_RECOVERING event.
Do you mean to say, PMD should set the data path pointers after calling the
call back function?
The PMD is running in the context of multiple EAL threads. How do these
threads synchronize such that only one thread sets these data pointers?
As I understand this event callback supposed to be called in the context of EAL
interrupt thread (whoever is more familiar with original idea, feel free to
correct
me if I missed something).
I could not figure this out. It looks to be called from the data plane thread
context.
I also have a thought on alternate design at the end, appreciate if you can
take a look.
How it is going to signal data-path threads that they need to stop/suspend
calling data-path API - that's I suppose is left to application to decide...
Same as right now it is application responsibility to stop data-path threads
before doing dev_stop()/dev/_config()/etc.
Ok, good, this expectation is not new. The application must have a mechanism
already.
2. The application should stop data path API invocation when process
the RTE_ETH_EVENT_ERR_RECOVERING event.
Any thoughts on how an application can do this?
We can ignore this question as there is already similar expectation set for
earlier functionalities.
3. The PMD should set the data path pointers to valid functions before
report RTE_ETH_EVENT_RECOVERY_SUCCESS event.
4. The application should enable data path API invocation when process
the RTE_ETH_EVENT_RECOVERY_SUCCESS event.
Do you mean to say that the application should not call the datapath APIs
while the PMD is running the recovery process?
Yes, I believe that's the intention.
Ok, this is good and makes sense.
Also, this patch introduce a driver internal function
rte_eth_fp_ops_setup which used as an help function for PMD.
[1]
http://patchwork.dpdk.org/project/dpdk/patch/20230220060839.1267349-2
-
ashok.k.kal...@intel.com/
Fixes: eb0d471a8941 ("ethdev: add proactive error handling mode")
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
---
doc/guides/prog_guide/poll_mode_drv.rst | 20 +++-
lib/ethdev/ethdev_driver.c | 8 +++
lib/ethdev/ethdev_driver.h | 10
lib/ethdev/rte_ethdev.h | 32 +++--
lib/ethdev/version.map | 1 +
5 files changed, 46 insertions(+), 25 deletions(-)
diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
b/doc/guides/prog_guide/poll_mode_drv.rst
index c145a9066c..e380ff135a 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -638,14 +638,9 @@ different from the application invokes recovery
in PASSIVE mode, the PMD automatically recovers from error in
PROACTIVE mode, and only a small amount of work is required for the
application.
-During error detection and automatic recovery, -the PMD sets the
data path pointers to dummy functions -(which will prevent the
crash), -and also make sure the control path operations fail with a return
code ``-EBUSY``.
-
-Because the PMD recovers automatically, -the application can only
sense that the data flow is disconnected for a while -and the control
API returns an error in this period.
+During error detection and automatic recovery, the PMD sets the data
+path pointers to dummy functions and also make sure the control path
+operations failed with a return code ``-EBUSY``.
In order to sense the error happening/recovering, as well as to
restore some additional configuration, @@ -653,9 +648,9 @@ three events
are available:
``RTE_ETH_EVENT_ERR_RECOVERING``
Notify the application that an error is detected
- and the recovery is being started.
+ and the recovery is about to start.
Upon receiving the event, the application should not invoke
- any control path function until receiving
+ any control and data path API until receiving
``RTE_ETH_EVENT_RECOVERY_SUCCESS`` or
``RTE_ETH_EVENT_RECOVERY_FAILED`` event.
.. note::
@@ -666,8 +661,9 @@ three events are available:
``RTE_ETH_EVENT_RECOVERY_SUCCESS``
Notify the application that the recovery from error is successful,
- the PMD already re-configures the port,
- and the effect is the same as a restart operation.
+ the PMD already re-configures the port.
+ The application should restore some a
Re: [PATCH 4/5] net/bnxt: use fp ops setup function
03/03/2023 01:38, fengchengwen пишет: On 2023/3/2 20:30, Konstantin Ananyev wrote: Use rte_eth_fp_ops_setup() instead of directly manipulating rte_eth_fp_ops variable. Cc: sta...@dpdk.org Signed-off-by: Chengwen Feng --- drivers/net/bnxt/bnxt_cpr.c| 5 + drivers/net/bnxt/bnxt_ethdev.c | 5 + 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c index 3950840600..a3f33c24c3 100644 --- a/drivers/net/bnxt/bnxt_cpr.c +++ b/drivers/net/bnxt/bnxt_cpr.c @@ -416,10 +416,7 @@ void bnxt_stop_rxtx(struct rte_eth_dev *eth_dev) eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy; eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy; I am not that familiar with bnxt driver, but shouldn't we set here other optional fp_ops (descripto_status, etc.) to some dummy values OR to null values? I checked the bnxt PMD code, the other fp_ops (rx_queue_count/rx_descriptor_status/tx_descriptor_status) both add following logic at the beginning of function: rc = is_bnxt_in_error(bp); if (rc) return rc; So I think it okey to keep it. I still think it is much safer/cleaner to update all fp_ops in one go (use fp_ops_reset()) here. But as you believe it would work either way, I'll leave it to bnxt maintainers to decide. - rte_eth_fp_ops[eth_dev->data->port_id].rx_pkt_burst = - eth_dev->rx_pkt_burst; - rte_eth_fp_ops[eth_dev->data->port_id].tx_pkt_burst = - eth_dev->tx_pkt_burst; + rte_eth_fp_ops_setup(eth_dev); rte_mb(); /* Allow time for threads to exit the real burst functions. */ diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c index 4083a69d02..d6064ceea4 100644 --- a/drivers/net/bnxt/bnxt_ethdev.c +++ b/drivers/net/bnxt/bnxt_ethdev.c @@ -4374,10 +4374,7 @@ static void bnxt_dev_recover(void *arg) if (rc) goto err_start; - rte_eth_fp_ops[bp->eth_dev->data->port_id].rx_pkt_burst = - bp->eth_dev->rx_pkt_burst; - rte_eth_fp_ops[bp->eth_dev->data->port_id].tx_pkt_burst = - bp->eth_dev->tx_pkt_burst; + rte_eth_fp_ops_setup(bp->eth_dev); rte_mb(); PMD_DRV_LOG(INFO, "Port: %u Recovered from FW reset\n", -- 2.17.1 .
Re: [EXT] Re: [PATCH v11 1/4] lib: add generic support for reading PMU events
Add support for programming PMU counters and reading their values
in
runtime bypassing kernel completely.
This is especially useful in cases where CPU cores are isolated i.e
run dedicated tasks. In such cases one cannot use standard perf
utility without sacrificing latency and performance.
Signed-off-by: Tomasz Duszynski
Acked-by: Morten Brørup
[...]
+int
+__rte_pmu_enable_group(void)
+{
+ struct rte_pmu_event_group *group = &RTE_PER_LCORE(_event_group);
+ int ret;
+
+ if (rte_pmu.num_group_events == 0)
+ return -ENODEV;
+
+ ret = open_events(group);
+ if (ret)
+ goto out;
+
+ ret = mmap_events(group);
+ if (ret)
+ goto out;
+
+ if (ioctl(group->fds[0], PERF_EVENT_IOC_RESET,
PERF_IOC_FLAG_GROUP) == -
1) {
+ ret = -errno;
+ goto out;
+ }
+
+ if (ioctl(group->fds[0], PERF_EVENT_IOC_ENABLE,
PERF_IOC_FLAG_GROUP) ==
-1) {
+ ret = -errno;
+ goto out;
+ }
+
+ rte_spinlock_lock(&rte_pmu.lock);
+ TAILQ_INSERT_TAIL(&rte_pmu.event_group_list, group, next);
Hmm.. so we insert pointer to TLS variable into the global list?
Wonder what would happen if that thread get terminated?
Nothing special. Any pointers to that thread-local in that thread are
invalided.
Can memory from its TLS block get re-used (by other thread or for
other
purposes)?
Why would any other thread reuse that?
Eventually main thread will need that data to do the cleanup.
I understand that main thread would need to access that data.
I am not sure that it would be able to.
Imagine thread calls rte_pmu_read(...) and then terminates, while
program
continues to run.
Is the example you describe here (i.e. a thread terminating in the middle
of
doing something) really a scenario DPDK is supposed to
support?
I am not talking about some abnormal termination.
Then I misunderstood your example; I thought you meant the tread was
terminated while inside the rte_pmu_read() function.
We do have ability to spawn control threads, user can spawn his own thread,
all these
threads can have limited life-time.
Not to mention about rte_thread_register()/rte_thread_unregister().
I agree that normal thread termination should be supported.
As I understand address of its RTE_PER_LCORE(_event_group) will still
remain
in rte_pmu.event_group_list,
even if it is probably not valid any more.
There should be a "destructor/done/finish" function available to remove
this
from the list.
[...]
Even if we'd decide to keep rte_pmu_read() as static inline (still
not
sure it is a good idea),
We want to save as much cpu cycles as we possibly can and inlining
does
helps
in that matter.
Ok, so asking same question from different thread: how many cycles it
will
save?
What is the difference in terms of performance when you have this
function
inlined vs not inlined?
We expect to use this in our in-house profiler library. For this reason, I
have a very strong preference for absolute maximum
performance.
Reading PMU events is for performance profiling, so I expect other
potential
users of the PMU library to share my opinion on this.
Well, from my perspective 14 cycles are not that much...
For reference, the i40e testpmd per-core performance report shows that it uses
36 cycles per packet.
This is a total of 1152 cycles per burst of 32 packets. 14 cycles overhead per
burst / 1152 cycles per burst = 1.2 % overhead.
But that is not all: If the application's pipeline has three stages, where the
PMU counters are read for each stage, the per-invocation overhead of 14 cycles
adds up, and the overhead per burst is now 3 * 14 / 1152 = 3.6 %.
I was too fast on the keyboard here... If the application does more work than
testpmd, it certainly also uses more than 1152 cycles to do that work. So
please ignore the 3.6 % as a wild exaggeration from an invalid example, and
just stick with the 1.2 % overhead - which I still consider significant, and
thus worth avoiding.
Wonder can we do both - hide struct rte_pmu_event_group from public API
and have inline function to read pmu stats?
if we can add a separate function that will allow user to get
struct perf_event_mmap_page * for given event index (or event name),
then later user can use
__rte_pmu_read_userpage(struct perf_event_mmap_page *pc)
directly.
Generalizing...
In my example here, the same function with 14 wasted cycles is called three
times. It might as well be three individual libraries each wasting 14 cycles
in its individual fast path processing function, due to a similarly relaxed
attitude regarding wasting 14 cycles.
My point is:
Real applications do much more work than testpmd, so all this "insignificant"
extra overhead in the libraries adds up!
Generally, I would like the DPDK Project to remain loyal to its original
philosophy, where performance is considered a Key Performance
Re: [PATCH] lib/bpf: Rename 'bpf_validate' to avoid potential conflict with libpcap
On Fri, 3 Mar 2023 21:56:54 +0800 Martzki wrote: > The library libpcap has their function 'bpf_validate' either so > there would be a multiple definition issue when linking with > librte_bpf.a and libpcap.a staticly. > > You can reproduce this issue by 'meson build -Dprefer_static=true > -Denable_apps=test-pmd -Denable_drivers=net/af_xdp,net/af_packet'. > Notice you need to have a static version of libpcap to reproduce this. > > In 2019 there was a patch reported the same issue but not applied: > https://inbox.dpdk.org/stable/2601191342ceee43887bde71ab9772580148a95...@irsmsx105.ger.corp.intel.com/T > > Since 'bpf_validate' is an internal function, I think adding an 'rte' > prefix is not a good idea and rename it to 'bpf_do_validate' instead. > > Signed-off-by: Martzki Let's change all the function names here to rte_bpf_XXX.
Re: [PATCH] lib/bpf: Rename 'bpf_validate' to avoid potential conflict with libpcap
On Sun, 5 Mar 2023 09:16:55 -0800
Stephen Hemminger wrote:
> On Fri, 3 Mar 2023 21:56:54 +0800
> Martzki wrote:
>
> > The library libpcap has their function 'bpf_validate' either so
> > there would be a multiple definition issue when linking with
> > librte_bpf.a and libpcap.a staticly.
> >
> > You can reproduce this issue by 'meson build -Dprefer_static=true
> > -Denable_apps=test-pmd -Denable_drivers=net/af_xdp,net/af_packet'.
> > Notice you need to have a static version of libpcap to reproduce this.
> >
> > In 2019 there was a patch reported the same issue but not applied:
> > https://inbox.dpdk.org/stable/2601191342ceee43887bde71ab9772580148a95...@irsmsx105.ger.corp.intel.com/T
> >
> > Since 'bpf_validate' is an internal function, I think adding an 'rte'
> > prefix is not a good idea and rename it to 'bpf_do_validate' instead.
> >
> > Signed-off-by: Martzki
>
> Let's change all the function names here to rte_bpf_XXX.
Something like this
diff --git a/lib/bpf/bpf.c b/lib/bpf/bpf.c
index 1e1dd42a589f..f218a8f2b049 100644
--- a/lib/bpf/bpf.c
+++ b/lib/bpf/bpf.c
@@ -31,14 +31,14 @@ rte_bpf_get_jit(const struct rte_bpf *bpf, struct
rte_bpf_jit *jit)
}
int
-bpf_jit(struct rte_bpf *bpf)
+rte_bpf_jit(struct rte_bpf *bpf)
{
int32_t rc;
#if defined(RTE_ARCH_X86_64)
- rc = bpf_jit_x86(bpf);
+ rc = rte_bpf_jit_x86(bpf);
#elif defined(RTE_ARCH_ARM64)
- rc = bpf_jit_arm64(bpf);
+ rc = rte_bpf_jit_arm64(bpf);
#else
rc = -ENOTSUP;
#endif
diff --git a/lib/bpf/bpf_convert.c b/lib/bpf/bpf_convert.c
index 9563274c9c6b..d441be66634f 100644
--- a/lib/bpf/bpf_convert.c
+++ b/lib/bpf/bpf_convert.c
@@ -23,11 +23,8 @@
#include
#include
-/* Workaround name conflicts with libpcap */
-#define bpf_validate(f, len) bpf_validate_libpcap(f, len)
#include
#include
-#undef bpf_validate
#include "bpf_impl.h"
#include "bpf_def.h"
diff --git a/lib/bpf/bpf_impl.h b/lib/bpf/bpf_impl.h
index b4d8e87c6dfb..7fca2db9bef6 100644
--- a/lib/bpf/bpf_impl.h
+++ b/lib/bpf/bpf_impl.h
@@ -17,12 +17,10 @@ struct rte_bpf {
uint32_t stack_sz;
};
-extern int bpf_validate(struct rte_bpf *bpf);
-
-extern int bpf_jit(struct rte_bpf *bpf);
-
-extern int bpf_jit_x86(struct rte_bpf *);
-extern int bpf_jit_arm64(struct rte_bpf *);
+extern int rte_bpf_validate(struct rte_bpf *bpf);
+extern int rte_bpf_jit(struct rte_bpf *bpf);
+extern int rte_bpf_jit_x86(struct rte_bpf *);
+extern int rte_bpf_jit_arm64(struct rte_bpf *);
extern int rte_bpf_logtype;
diff --git a/lib/bpf/bpf_jit_x86.c b/lib/bpf/bpf_jit_x86.c
index c1a30e038660..182004ac7d6c 100644
--- a/lib/bpf/bpf_jit_x86.c
+++ b/lib/bpf/bpf_jit_x86.c
@@ -1490,7 +1490,7 @@ emit(struct bpf_jit_state *st, const struct rte_bpf *bpf)
* produce a native ISA version of the given BPF code.
*/
int
-bpf_jit_x86(struct rte_bpf *bpf)
+rte_bpf_jit_x86(struct rte_bpf *bpf)
{
int32_t rc;
uint32_t i;
diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
index 1e17df6ce0ab..2c4bca358603 100644
--- a/lib/bpf/bpf_load.c
+++ b/lib/bpf/bpf_load.c
@@ -108,9 +108,9 @@ rte_bpf_load(const struct rte_bpf_prm *prm)
return NULL;
}
- rc = bpf_validate(bpf);
+ rc = rte_bpf_validate(bpf);
if (rc == 0) {
- bpf_jit(bpf);
+ rte_bpf_jit(bpf);
if (mprotect(bpf, bpf->sz, PROT_READ) != 0)
rc = -ENOMEM;
}
diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
index 61cbb42216b8..2d3d899966b9 100644
--- a/lib/bpf/bpf_validate.c
+++ b/lib/bpf/bpf_validate.c
@@ -2302,7 +2302,7 @@ evaluate(struct bpf_verifier *bvf)
}
int
-bpf_validate(struct rte_bpf *bpf)
+rte_bpf_validate(struct rte_bpf *bpf)
{
int32_t rc;
struct bpf_verifier bvf;
[PATCH] app/test: fix data length of each packet segment
Assign correct data length to each segments according
to the given pkt_len and nb_pkt_segs, instead of
using pkt_len as the data_len of every packet segment
Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
Cc: cunming.li...@intel.com
Signed-off-by: Zhuobin Huang
---
app/test/packet_burst_generator.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
index 6b42b9b83b..c2e51a3bf1 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -350,6 +350,8 @@ generate_packet_burst_proto(struct rte_mempool *mp,
struct rte_mbuf *pkt_seg;
struct rte_mbuf *pkt;
+ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
+
for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
pkt = rte_pktmbuf_alloc(mp);
if (pkt == NULL) {
@@ -359,7 +361,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
break;
}
- pkt->data_len = pkt_len;
+ pkt->data_len = pkt_seg_data_len;
pkt_seg = pkt;
for (i = 1; i < nb_pkt_segs; i++) {
pkt_seg->next = rte_pktmbuf_alloc(mp);
@@ -369,7 +371,10 @@ generate_packet_burst_proto(struct rte_mempool *mp,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- pkt_seg->data_len = pkt_len;
+ if(i != nb_pkt_segs - 1)
+ pkt_seg->data_len = pkt_seg_data_len;
+ else
+ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
}
pkt_seg->next = NULL; /* Last segment of packet. */
--
2.34.1
[PATCH v2] app/test: fix data length of each packet segment
v2:
* fix code format
Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
Cc: cunming.li...@intel.com
Signed-off-by: Zhuobin Huang
---
...x-data-length-of-each-packet-segment.patch | 54 +++
app/test/packet_burst_generator.c | 2 +-
2 files changed, 55 insertions(+), 1 deletion(-)
create mode 100644 0001-app-test-fix-data-length-of-each-packet-segment.patch
diff --git a/0001-app-test-fix-data-length-of-each-packet-segment.patch
b/0001-app-test-fix-data-length-of-each-packet-segment.patch
new file mode 100644
index 00..aa52bb2ae2
--- /dev/null
+++ b/0001-app-test-fix-data-length-of-each-packet-segment.patch
@@ -0,0 +1,54 @@
+From 7b3827259bded81127fb33572868815bd53ad564 Mon Sep 17 00:00:00 2001
+From: Zhuobin Huang
+Date: Mon, 6 Mar 2023 04:13:20 +0800
+Subject: [PATCH] app/test: fix data length of each packet segment
+
+Assign correct data length to each segments according
+to the given pkt_len and nb_pkt_segs, instead of
+using pkt_len as the data_len of every packet segment
+
+Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
+Cc: cunming.li...@intel.com
+
+Signed-off-by: Zhuobin Huang
+---
+ app/test/packet_burst_generator.c | 9 +++--
+ 1 file changed, 7 insertions(+), 2 deletions(-)
+
+diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
+index 6b42b9b83b..c2e51a3bf1 100644
+--- a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
+@@ -350,6 +350,8 @@ generate_packet_burst_proto(struct rte_mempool *mp,
+ struct rte_mbuf *pkt_seg;
+ struct rte_mbuf *pkt;
+
++ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
++
+ for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
+ pkt = rte_pktmbuf_alloc(mp);
+ if (pkt == NULL) {
+@@ -359,7 +361,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
+ break;
+ }
+
+- pkt->data_len = pkt_len;
++ pkt->data_len = pkt_seg_data_len;
+ pkt_seg = pkt;
+ for (i = 1; i < nb_pkt_segs; i++) {
+ pkt_seg->next = rte_pktmbuf_alloc(mp);
+@@ -369,7 +371,10 @@ generate_packet_burst_proto(struct rte_mempool *mp,
+ goto nomore_mbuf;
+ }
+ pkt_seg = pkt_seg->next;
+- pkt_seg->data_len = pkt_len;
++ if(i != nb_pkt_segs - 1)
++ pkt_seg->data_len = pkt_seg_data_len;
++ else
++ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
+ }
+ pkt_seg->next = NULL; /* Last segment of packet. */
+
+--
+2.34.1
+
diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
index c2e51a3bf1..940f6ddde8 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -371,7 +371,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- if(i != nb_pkt_segs - 1)
+ if (i != nb_pkt_segs - 1)
pkt_seg->data_len = pkt_seg_data_len;
else
pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
--
2.34.1
[PATCH] app/test: fix data length of each packet segment
Assign correct data length to each segments according
to the given pkt_len and nb_pkt_segs, instead of
using pkt_len as the data_len of every packet segment
Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
Cc: cunming.li...@intel.com
Signed-off-by: Zhuobin Huang
---
app/test/packet_burst_generator.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
index 6b42b9b83b..940f6ddde8 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -350,6 +350,8 @@ generate_packet_burst_proto(struct rte_mempool *mp,
struct rte_mbuf *pkt_seg;
struct rte_mbuf *pkt;
+ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
+
for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
pkt = rte_pktmbuf_alloc(mp);
if (pkt == NULL) {
@@ -359,7 +361,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
break;
}
- pkt->data_len = pkt_len;
+ pkt->data_len = pkt_seg_data_len;
pkt_seg = pkt;
for (i = 1; i < nb_pkt_segs; i++) {
pkt_seg->next = rte_pktmbuf_alloc(mp);
@@ -369,7 +371,10 @@ generate_packet_burst_proto(struct rte_mempool *mp,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- pkt_seg->data_len = pkt_len;
+ if (i != nb_pkt_segs - 1)
+ pkt_seg->data_len = pkt_seg_data_len;
+ else
+ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
}
pkt_seg->next = NULL; /* Last segment of packet. */
--
2.34.1
[PATCH] app/test: fix data length of each packet segment
Assign correct data length to each segments according
to the given pkt_len and nb_pkt_segs, instead of
using pkt_len as the data_len of every packet segment
Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
Cc: cunming.li...@intel.com
Signed-off-by: Zhuobin Huang
---
.mailmap | 1 +
app/test/packet_burst_generator.c | 9 +++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/.mailmap b/.mailmap
index a9f4f28fba..9128fde9c5 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1576,6 +1576,7 @@ Zhipeng Lu
Zhirun Yan
Zhiwei He
Zhiyong Yang
+Zhuobin Huang
Zi Hu
Zijie Pan
Ziyang Xuan
diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
index 6b42b9b83b..940f6ddde8 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -350,6 +350,8 @@ generate_packet_burst_proto(struct rte_mempool *mp,
struct rte_mbuf *pkt_seg;
struct rte_mbuf *pkt;
+ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
+
for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
pkt = rte_pktmbuf_alloc(mp);
if (pkt == NULL) {
@@ -359,7 +361,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
break;
}
- pkt->data_len = pkt_len;
+ pkt->data_len = pkt_seg_data_len;
pkt_seg = pkt;
for (i = 1; i < nb_pkt_segs; i++) {
pkt_seg->next = rte_pktmbuf_alloc(mp);
@@ -369,7 +371,10 @@ generate_packet_burst_proto(struct rte_mempool *mp,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- pkt_seg->data_len = pkt_len;
+ if (i != nb_pkt_segs - 1)
+ pkt_seg->data_len = pkt_seg_data_len;
+ else
+ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
}
pkt_seg->next = NULL; /* Last segment of packet. */
--
2.34.1
Re: [PATCH 1/5] ethdev: fix race-condition of proactive error handling mode
On 2023/3/4 0:51, Ferruh Yigit wrote:
> On 3/2/2023 12:08 PM, Konstantin Ananyev wrote:
>>
>>> In the proactive error handling mode, the PMD will set the data path
>>> pointers to dummy functions and then try recovery, in this period the
>>> application may still invoking data path API. This will introduce a
>>> race-condition with data path which may lead to crash [1].
>>>
>>> Although the PMD added delay after setting data path pointers to cover
>>> the above race-condition, it reduces the probability, but it doesn't
>>> solve the problem.
>>>
>>> To solve the race-condition problem fundamentally, the following
>>> requirements are added:
>>> 1. The PMD should set the data path pointers to dummy functions after
>>>report RTE_ETH_EVENT_ERR_RECOVERING event.
>>> 2. The application should stop data path API invocation when process
>>>the RTE_ETH_EVENT_ERR_RECOVERING event.
>>> 3. The PMD should set the data path pointers to valid functions before
>>>report RTE_ETH_EVENT_RECOVERY_SUCCESS event.
>>> 4. The application should enable data path API invocation when process
>>>the RTE_ETH_EVENT_RECOVERY_SUCCESS event.
>>>
>
> How this is solving the race-condition, by pushing responsibility to
> stop data path to application?
Yes, I think it's more practical to collaborate with application.
The application will control API invocation (including control and data path),
>From a DPDK SDK perspective, it has a God perspective.
>
> What if application is not interested in recovery modes at all and not
> registered any callback for the recovery?
There's probably race-condition which may lead to crash, because DPDK worker
threads runs busyloop and located on isolated core, and also PMDs add delay
time,
the actual probability of occurence is very very low, at least for HNS3 pmd it
has not run out for at least four years.
>
> I think driver should not rely on application for this, unless
> application explicitly says (to driver) that it is handling recovery,
If application register the event callback, the PMD could deduce that
application will know this.
If application not register, then PMD will recovery itself and maybe
race-condition.
> right now there is no way for driver to know this.
>
>
>>> Also, this patch introduce a driver internal function
>>> rte_eth_fp_ops_setup which used as an help function for PMD.
>>>
>>> [1]
>>> http://patchwork.dpdk.org/project/dpdk/patch/20230220060839.1267349-2-ashok.k.kal...@intel.com/
>>>
>>> Fixes: eb0d471a8941 ("ethdev: add proactive error handling mode")
>>> Cc: sta...@dpdk.org
>>>
>>> Signed-off-by: Chengwen Feng
>>> ---
>>> doc/guides/prog_guide/poll_mode_drv.rst | 20 +++-
>>> lib/ethdev/ethdev_driver.c | 8 +++
>>> lib/ethdev/ethdev_driver.h | 10
>>> lib/ethdev/rte_ethdev.h | 32 +++--
>>> lib/ethdev/version.map | 1 +
>>> 5 files changed, 46 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
>>> b/doc/guides/prog_guide/poll_mode_drv.rst
>>> index c145a9066c..e380ff135a 100644
>>> --- a/doc/guides/prog_guide/poll_mode_drv.rst
>>> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
>>> @@ -638,14 +638,9 @@ different from the application invokes recovery in
>>> PASSIVE mode,
>>> the PMD automatically recovers from error in PROACTIVE mode,
>>> and only a small amount of work is required for the application.
>>>
>>> -During error detection and automatic recovery,
>>> -the PMD sets the data path pointers to dummy functions
>>> -(which will prevent the crash),
>>> -and also make sure the control path operations fail with a return code
>>> ``-EBUSY``.
>>> -
>>> -Because the PMD recovers automatically,
>>> -the application can only sense that the data flow is disconnected for a
>>> while
>>> -and the control API returns an error in this period.
>>> +During error detection and automatic recovery, the PMD sets the data path
>>> +pointers to dummy functions and also make sure the control path operations
>>> +failed with a return code ``-EBUSY``.
>>>
>>> In order to sense the error happening/recovering,
>>> as well as to restore some additional configuration,
>>> @@ -653,9 +648,9 @@ three events are available:
>>>
>>> ``RTE_ETH_EVENT_ERR_RECOVERING``
>>> Notify the application that an error is detected
>>> - and the recovery is being started.
>>> + and the recovery is about to start.
>>> Upon receiving the event, the application should not invoke
>>> - any control path function until receiving
>>> + any control and data path API until receiving
>>> ``RTE_ETH_EVENT_RECOVERY_SUCCESS`` or ``RTE_ETH_EVENT_RECOVERY_FAILED``
>>> event.
>>>
>>> .. note::
>>> @@ -666,8 +661,9 @@ three events are available:
>>>
>>> ``RTE_ETH_EVENT_RECOVERY_SUCCESS``
>>> Notify the application that the recovery from error is successful,
>>> - the PMD already re-configures the port,
>>> - and the effect
Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup
On 2023/3/4 1:19, Ferruh Yigit wrote:
> On 2/26/2023 5:22 PM, Konstantin Ananyev wrote:
>>
If ethdev enqueue or dequeue function is called during
eth_dev_fp_ops_setup(), it may get pre-empted after setting the
function pointers, but before setting the pointer to port data.
In this case the newly registered enqueue/dequeue function will
use dummy port data and end up in seg fault.
This patch moves the updation of each data pointers before
updating corresponding function pointers.
Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate
structure")
Cc: sta...@dpdk.org
>
> Why is something calling enqueue/dequeue when device is not fully
>>> started.
> A correctly written application would not call rx/tx burst until
> after ethdev start had finished.
Please refer the eb0d471a894 (ethdev: add proactive error handling
mode), when driver recover itself, the application may still invoke
>>> enqueue/dequeue API.
>>>
>>> Right now DPDK ethdev layer *does not* provide synchronization
>>> mechanisms between data-path and control-path functions.
>>> That was a deliberate deisgn choice. If we want to change that
>>> rule, then I
>>> suppose we need a community consensus for it.
>>> I think that if the driver wants to provide some sort of error
>>> recovery
>>> procedure, then it has to provide some synchronization mechanism
>>> inside it
>>> between data-path and control-path functions.
>>> Actually looking at eb0d471a894 (ethdev: add proactive error handling
>>> mode), and following patches I wonder how it creeped in?
>>> It seems we just introduced a loophole for race condition with this
>>> approach...
>
> Could you try to describe the specific scenario of loophole ?
Ok, as I understand the existing mechanism:
When PMD wants to start a recovery it has to:
- invoke rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING);
That supposed to call user provided callback. After callback is
finished PMD assumes
that user is aware that recovery is about to start and should
make some precautions.
- when recovery is finished it invokes another callback:
RTE_ETH_EVENT_RECOVERY_(SUCCESS/FAILED). After that user either
can continue to
use port or have to treat is as faulty.
The idea is ok in principle, but there is a problem.
lib/ethdev/rte_ethdev.h:
/** Port recovering from a hardware or firmware error.
* If PMD supports proactive error recovery,
* it should trigger this event to notify application
* that it detected an error and the recovery is being started.
<<< !
* Upon receiving the event, the application should not
invoke any control path API
* (such as rte_eth_dev_configure/rte_eth_dev_stop...) until
receiving
* RTE_ETH_EVENT_RECOVERY_SUCCESS or
RTE_ETH_EVENT_RECOVERY_FAILED event.
* The PMD will set the data path pointers to dummy functions,
* and re-set the data path pointers to non-dummy functions
* before reporting RTE_ETH_EVENT_RECOVERY_SUCCESS event.
<<< !
That part is just wrong I believe.
It should be:
Upon receiving the event, the application should not invoke any *both
control and data-path* API
until receiving RTE_ETH_EVENT_RECOVERY_SUCCESS or
RTE_ETH_EVENT_RECOVERY_FAILED event.
Resetting data path pointers to dummy functions by PMD *before* invoking
rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING);
introduces a race-condition with data-path threads, as such thread
could already be inside RX/TX function
or can already read RX/TX function/data pointers and be about to use
them.
>>>
>>> Current practices: the PMDs already add some delay after set Rx/Tx
>>> callback to dummy, and plus the DPDK
>>> worker thread is busypolling, the probability of occurence in reality
>>> is zero. But in theoretically exist
>>> the above race-condition.
>>
>>
>> Adding delay might make a problem a bit less reproducible,
>> but it doesn't fix it.
>> The bug is still there.
>>
>>
>>>
And right now rte_ethdev layer doesn't provide any mechanism to check
it or wait when they'll finish, etc.
>>>
>>> Yes
>>>
So, probably the simplest way to fix it with existing DPDK design:
- user level callback RTE_ETH_EVENT_ERR_RECOVERING should return
only after it ensures that *all*
application threads (and processes) stopped using either control
or data-path functions for that port
>>>
>>> Agree
>>>
(yes it means that application that wants to
Re: [PATCH 4/5] net/bnxt: use fp ops setup function
On Sun, Mar 5, 2023 at 7:58 AM Konstantin Ananyev wrote: > > 03/03/2023 01:38, fengchengwen пишет: > > On 2023/3/2 20:30, Konstantin Ananyev wrote: > >> > >>> Use rte_eth_fp_ops_setup() instead of directly manipulating > >>> rte_eth_fp_ops variable. > >>> > >>> Cc: sta...@dpdk.org > >>> > >>> Signed-off-by: Chengwen Feng > >>> --- > >>> drivers/net/bnxt/bnxt_cpr.c| 5 + > >>> drivers/net/bnxt/bnxt_ethdev.c | 5 + > >>> 2 files changed, 2 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c > >>> index 3950840600..a3f33c24c3 100644 > >>> --- a/drivers/net/bnxt/bnxt_cpr.c > >>> +++ b/drivers/net/bnxt/bnxt_cpr.c > >>> @@ -416,10 +416,7 @@ void bnxt_stop_rxtx(struct rte_eth_dev *eth_dev) > >>> eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy; > >>> eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy; > >> > >> I am not that familiar with bnxt driver, but shouldn't we set here > >> other optional fp_ops (descripto_status, etc.) to some dummy values OR to > >> null values? > > > > I checked the bnxt PMD code, the other fp_ops > > (rx_queue_count/rx_descriptor_status/tx_descriptor_status) > > both add following logic at the beginning of function: > > > > rc = is_bnxt_in_error(bp); > > if (rc) > > return rc; > > > > So I think it okey to keep it. > > I still think it is much safer/cleaner to update all fp_ops in one go > (use fp_ops_reset()) here. > But as you believe it would work either way, I'll leave it to bnxt > maintainers to decide. We have been operating without the application being aware of the underlying functionality for some time now. Each step here is an improvement. I think it is okay to keep it simple and update them separately. Thanks Ajit > > > > > >> > >>> > >>> - rte_eth_fp_ops[eth_dev->data->port_id].rx_pkt_burst = > >>> - eth_dev->rx_pkt_burst; > >>> - rte_eth_fp_ops[eth_dev->data->port_id].tx_pkt_burst = > >>> - eth_dev->tx_pkt_burst; > >>> + rte_eth_fp_ops_setup(eth_dev); > >>> rte_mb(); > >>> > >>> /* Allow time for threads to exit the real burst functions. */ > >>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c > >>> b/drivers/net/bnxt/bnxt_ethdev.c > >>> index 4083a69d02..d6064ceea4 100644 > >>> --- a/drivers/net/bnxt/bnxt_ethdev.c > >>> +++ b/drivers/net/bnxt/bnxt_ethdev.c > >>> @@ -4374,10 +4374,7 @@ static void bnxt_dev_recover(void *arg) > >>> if (rc) > >>> goto err_start; > >>> > >>> - rte_eth_fp_ops[bp->eth_dev->data->port_id].rx_pkt_burst = > >>> - bp->eth_dev->rx_pkt_burst; > >>> - rte_eth_fp_ops[bp->eth_dev->data->port_id].tx_pkt_burst = > >>> - bp->eth_dev->tx_pkt_burst; > >>> + rte_eth_fp_ops_setup(bp->eth_dev); > >>> rte_mb(); > >>> > >>> PMD_DRV_LOG(INFO, "Port: %u Recovered from FW reset\n", > >>> -- > >>> 2.17.1 > >> > >> . > >> > smime.p7s Description: S/MIME Cryptographic Signature
RE: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup
> -Original Message-
> From: Ferruh Yigit
> Sent: Saturday, March 4, 2023 1:19 AM
> To: Konstantin Ananyev ; dev@dpdk.org;
> fengchengwen
> ; Konstantin Ananyev
> ; Honnappa
> Nagarahalli ; Stephen Hemminger
> ;
> Ruifeng Wang ; Ajit Khaparde
> (ajit.khapa...@broadcom.com)
>
> Subject: Re: [PATCH 2/2] ethdev: fix race condition in fast-path ops setup
>
> On 2/26/2023 5:22 PM, Konstantin Ananyev wrote:
> >
> >>> If ethdev enqueue or dequeue function is called during
> >>> eth_dev_fp_ops_setup(), it may get pre-empted after setting
> >>> the function pointers, but before setting the pointer to port
> >>> data.
> >>> In this case the newly registered enqueue/dequeue function
> >>> will use dummy port data and end up in seg fault.
> >>>
> >>> This patch moves the updation of each data pointers before
> >>> updating corresponding function pointers.
> >>>
> >>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into
> >>> separate
> >>> structure")
> >>> Cc: sta...@dpdk.org
>
> Why is something calling enqueue/dequeue when device is not
> fully
> >> started.
> A correctly written application would not call rx/tx burst
> until after ethdev start had finished.
> >>>
> >>> Please refer the eb0d471a894 (ethdev: add proactive error
> >>> handling mode), when driver recover itself, the application may
> >>> still invoke
> >> enqueue/dequeue API.
> >>
> >> Right now DPDK ethdev layer *does not* provide synchronization
> >> mechanisms between data-path and control-path functions.
> >> That was a deliberate deisgn choice. If we want to change that
> >> rule, then I suppose we need a community consensus for it.
> >> I think that if the driver wants to provide some sort of error
> >> recovery procedure, then it has to provide some synchronization
> >> mechanism inside it between data-path and control-path functions.
> >> Actually looking at eb0d471a894 (ethdev: add proactive error
> >> handling mode), and following patches I wonder how it creeped in?
> >> It seems we just introduced a loophole for race condition with
> >> this approach...
>
> Could you try to describe the specific scenario of loophole ?
> >>>
> >>> Ok, as I understand the existing mechanism:
> >>>
> >>> When PMD wants to start a recovery it has to:
> >>> - invoke
> >>> rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING);
> >>> That supposed to call user provided callback. After callback is
> >>> finished PMD assumes
> >>> that user is aware that recovery is about to start and should
> >>> make some precautions.
> >>> - when recovery is finished it invokes another callback:
> >>> RTE_ETH_EVENT_RECOVERY_(SUCCESS/FAILED). After that user either
> >>> can continue to
> >>> use port or have to treat is as faulty.
> >>>
> >>> The idea is ok in principle, but there is a problem.
> >>>
> >>> lib/ethdev/rte_ethdev.h:
> >>> /** Port recovering from a hardware or firmware error.
> >>> * If PMD supports proactive error recovery,
> >>> * it should trigger this event to notify application
> >>> * that it detected an error and the recovery is being started.
> >>>
> >>> <<< !
> >>> * Upon receiving the event, the application should not
> >>> invoke any control path API
> >>> * (such as rte_eth_dev_configure/rte_eth_dev_stop...)
> >>> until receiving
> >>> * RTE_ETH_EVENT_RECOVERY_SUCCESS or
> >>> RTE_ETH_EVENT_RECOVERY_FAILED event.
> >>> * The PMD will set the data path pointers to dummy
> >>> functions,
> >>> * and re-set the data path pointers to non-dummy functions
> >>> * before reporting RTE_ETH_EVENT_RECOVERY_SUCCESS event.
> >>> <<< !
> >>>
> >>> That part is just wrong I believe.
> >>> It should be:
> >>> Upon receiving the event, the application should not invoke any
> >>> *both control and data-path* API until receiving
> >>> RTE_ETH_EVENT_RECOVERY_SUCCESS or RTE_ETH_EVENT_RECOVERY_FAILED
> >>> event.
> >>> Resetting data path pointers to dummy functions by PMD *before*
> >>> invoking rte_eth_dev_callback_process(RTE_ETH_EVENT_ERR_RECOVERING);
> >>> introduces a race-condition with data-path threads, as such thread
> >>> could already be inside RX/TX function or can already read RX/TX
> >>> function/data pointers and be about to use them.
> >>
> >> Current practices: the PMDs already add some delay after set Rx/Tx
> >> callback to dummy, and plus the DPDK worker thread is busypolling,
> >> the probability of occurence in reality is zero. But in theoretically
> >> exist the above race-condition.
> >
> >
> > Adding delay might make a problem a bit less reproducible, but it
> > doesn't fix it.
> > The bug is still there.
> >
> >
> >>
> >>> And right now rte_ethdev layer doesn't provide
RE: [PATCH v2] net/virtio: deduce IP length for Virtio TSO checksum
Hi Boleslav,
The change seems good, but patchwork is complaining about lack of .mailmap
change.
http://mails.dpdk.org/archives/test-report/2023-March/363061.html
Guess this is your first patch? So you need to add name and email in mailmap
file.
Thanks,
Chenbo
> -Original Message-
> From: Andrew Rybchenko
> Sent: Friday, March 3, 2023 11:14 PM
> To: Boleslav Stankevich ; dev@dpdk.org
> Cc: sta...@dpdk.org; Maxime Coquelin ; Xia,
> Chenbo
> Subject: Re: [PATCH v2] net/virtio: deduce IP length for Virtio TSO
> checksum
>
> Cc Maxime and Chenbo
>
> On 3/3/23 14:19, Boleslav Stankevich wrote:
> > The length of TSO payload could not fit into 16 bits provided by the
> > IPv4 total length and IPv6 payload length fields. Thus, deduce it
> > from the length of the packet.
> >
> > Fixes: 696573046e9 ("net/virtio: support TSO")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Boleslav Stankevich
> > Reviewed-by: Andrew Rybchenko
> > ---
> > drivers/net/virtio/virtio_rxtx.c | 25 -
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_rxtx.c
> b/drivers/net/virtio/virtio_rxtx.c
> > index 2d0afd3302..e48ff3cca7 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -404,29 +404,36 @@ virtio_tso_fix_cksum(struct rte_mbuf *m)
> > if (likely(rte_pktmbuf_data_len(m) >= m->l2_len + m->l3_len +
> > m->l4_len)) {
> > struct rte_ipv4_hdr *iph;
> > - struct rte_ipv6_hdr *ip6h;
> > struct rte_tcp_hdr *th;
> > - uint16_t prev_cksum, new_cksum, ip_len, ip_paylen;
> > + uint16_t prev_cksum, new_cksum;
> > + uint32_t ip_paylen;
> > uint32_t tmp;
> >
> > iph = rte_pktmbuf_mtod_offset(m,
> > struct rte_ipv4_hdr *, m->l2_len);
> > th = RTE_PTR_ADD(iph, m->l3_len);
> > +
> > + /*
> > +* Calculate IPv4 header checksum with current total length
> value
> > +* (whatever it is) to have correct checksum after update on
> edits
> > +* done by TSO.
> > +*/
> > if ((iph->version_ihl >> 4) == 4) {
> > iph->hdr_checksum = 0;
> > iph->hdr_checksum = rte_ipv4_cksum(iph);
> > - ip_len = iph->total_length;
> > - ip_paylen = rte_cpu_to_be_16(rte_be_to_cpu_16(ip_len) -
> > - m->l3_len);
> > - } else {
> > - ip6h = (struct rte_ipv6_hdr *)iph;
> > - ip_paylen = ip6h->payload_len;
> > }
> >
> > + /*
> > +* Do not use IPv4 total length and IPv6 payload length fields
> to get
> > +* TSO payload length since it could not fit into 16 bits.
> > +*/
> > + ip_paylen = rte_cpu_to_be_32(rte_pktmbuf_pkt_len(m) - m-
> >l2_len -
> > + m->l3_len);
> > +
> > /* calculate the new phdr checksum not including ip_paylen */
> > prev_cksum = th->cksum;
> > tmp = prev_cksum;
> > - tmp += ip_paylen;
> > + tmp += (ip_paylen & 0x) + (ip_paylen >> 16);
> > tmp = (tmp & 0x) + (tmp >> 16);
> > new_cksum = tmp;
> >
Re: [PATCH] examples/l3fwd-power: support CPPC cpufreq
Hi, Thomas, A gentle ping~ Since this patch has been acked by David Hunt, is there anything more I can do to push the process forward? Thanks, Jie Hai On 2023/2/22 17:46, Hunt, David wrote: On 22/02/2023 02:13, Jie Hai wrote: Hi, David Hunt, Kindly ping. Could you please take a look at this patch? Thanks, Jie Hai On 2023/1/31 10:58, Jie Hai wrote: Currently the l3fwd-power only supports ACPI cpufreq and Pstate cpufreq, This patch adds CPPC cpufreq. Signed-off-by: Jie Hai Hi, Jie Hai, Apologies, this patch never got to my inbox. Looks good to me. Acked-by: David Hunt .
[PATCH] app/test: fix data length of each packet segment
Assign correct data length to each segments according
to the given pkt_len and nb_pkt_segs, instead of
using pkt_len as the data_len of every packet segment
Fixes: a9c9e9698d5e ("app/test: allow to create packets of different sizes")
Cc: cunming.li...@intel.com
Signed-off-by: Zhuobin Huang
---
.mailmap | 1 +
app/test/packet_burst_generator.c | 18 ++
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/.mailmap b/.mailmap
index a9f4f28fba..9128fde9c5 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1576,6 +1576,7 @@ Zhipeng Lu
Zhirun Yan
Zhiwei He
Zhiyong Yang
+Zhuobin Huang
Zi Hu
Zijie Pan
Ziyang Xuan
diff --git a/app/test/packet_burst_generator.c
b/app/test/packet_burst_generator.c
index 6b42b9b83b..898b717073 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -269,6 +269,8 @@ generate_packet_burst(struct rte_mempool *mp, struct
rte_mbuf **pkts_burst,
struct rte_mbuf *pkt_seg;
struct rte_mbuf *pkt;
+ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
+
for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
pkt = rte_pktmbuf_alloc(mp);
if (pkt == NULL) {
@@ -278,7 +280,7 @@ generate_packet_burst(struct rte_mempool *mp, struct
rte_mbuf **pkts_burst,
break;
}
- pkt->data_len = pkt_len;
+ pkt->data_len = pkt_seg_data_len;
pkt_seg = pkt;
for (i = 1; i < nb_pkt_segs; i++) {
pkt_seg->next = rte_pktmbuf_alloc(mp);
@@ -288,7 +290,10 @@ generate_packet_burst(struct rte_mempool *mp, struct
rte_mbuf **pkts_burst,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- pkt_seg->data_len = pkt_len;
+ if (i != nb_pkt_segs - 1)
+ pkt_seg->data_len = pkt_seg_data_len;
+ else
+ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
}
pkt_seg->next = NULL; /* Last segment of packet. */
@@ -350,6 +355,8 @@ generate_packet_burst_proto(struct rte_mempool *mp,
struct rte_mbuf *pkt_seg;
struct rte_mbuf *pkt;
+ const uint8_t pkt_seg_data_len = pkt_len / nb_pkt_segs;
+
for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) {
pkt = rte_pktmbuf_alloc(mp);
if (pkt == NULL) {
@@ -359,7 +366,7 @@ generate_packet_burst_proto(struct rte_mempool *mp,
break;
}
- pkt->data_len = pkt_len;
+ pkt->data_len = pkt_seg_data_len;
pkt_seg = pkt;
for (i = 1; i < nb_pkt_segs; i++) {
pkt_seg->next = rte_pktmbuf_alloc(mp);
@@ -369,7 +376,10 @@ generate_packet_burst_proto(struct rte_mempool *mp,
goto nomore_mbuf;
}
pkt_seg = pkt_seg->next;
- pkt_seg->data_len = pkt_len;
+ if (i != nb_pkt_segs - 1)
+ pkt_seg->data_len = pkt_seg_data_len;
+ else
+ pkt_seg->data_len = pkt_seg_data_len + pkt_len
% nb_pkt_segs;
}
pkt_seg->next = NULL; /* Last segment of packet. */
--
2.34.1
RE: [PATCH v2] net/i40e: don't check link status on device start
> -Original Message-
> From: David Marchand
> Sent: Tuesday, December 13, 2022 5:19 PM
> To: dev@dpdk.org
> Cc: sta...@dpdk.org; Zhang, Yuying ; Xing, Beilei
> ; Zhang, Qi Z ; Dapeng Yu
> ; Wenxuan Wu ; Wang,
> Jie1X
> Subject: [PATCH v2] net/i40e: don't check link status on device start
>
> The mentioned changes broke existing applications when the link status of
> i40e ports is down at the time the port is started.
> Revert those changes, the original issue will need a different fix.
>
> Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level")
> Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722")
> Cc: sta...@dpdk.org
>
> Signed-off-by: David Marchand
> ---
> Changes since v1:
> - since the CI reports a failure on v1, simplified the fix by only
> reverting commits,
>
> ---
> drivers/net/i40e/i40e_ethdev.c | 50 +-
> 1 file changed, 7 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7726a89d99..a982e42264 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -387,7 +387,6 @@ static int i40e_set_default_mac_addr(struct
> rte_eth_dev *dev,
> struct rte_ether_addr *mac_addr);
>
> static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static
> void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size);
>
> static int i40e_ethertype_filter_convert(
> const struct rte_eth_ethertype_filter *input, @@ -1711,6 +1710,11 @@
> eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused)
>*/
> i40e_add_tx_flow_control_drop_filter(pf);
>
> + /* Set the max frame size to 0x2600 by default,
> + * in case other drivers changed the default value.
> + */
> + i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0,
> NULL);
> +
> /* initialize RSS rule list */
> TAILQ_INIT(&pf->rss_config_list);
>
> @@ -2328,7 +2332,6 @@ i40e_dev_start(struct rte_eth_dev *dev)
> uint32_t intr_vector = 0;
> struct i40e_vsi *vsi;
> uint16_t nb_rxq, nb_txq;
> - uint16_t max_frame_size;
>
> hw->adapter_stopped = 0;
>
> @@ -2467,9 +2470,6 @@ i40e_dev_start(struct rte_eth_dev *dev)
> "please call hierarchy_commit() "
> "before starting the port");
>
> - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
> - i40e_set_mac_max_frame(dev, max_frame_size);
> -
> return I40E_SUCCESS;
>
> tx_err:
> @@ -2809,9 +2809,6 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
> return i40e_phy_conf_link(hw, abilities, speed, false); }
>
> -#define CHECK_INTERVAL 100 /* 100ms */
> -#define MAX_REPEAT_TIME10 /* 1s (10 * 100ms) in total */
> -
> static __rte_always_inline void
> update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@
> -2878,6 +2875,8 @@ static __rte_always_inline void update_link_aq(struct
> i40e_hw *hw, struct rte_eth_link *link,
> bool enable_lse, int wait_to_complete) {
> +#define CHECK_INTERVAL 100 /* 100ms */
> +#define MAX_REPEAT_TIME10 /* 1s (10 * 100ms) in total */
> uint32_t rep_cnt = MAX_REPEAT_TIME;
> struct i40e_link_status link_status;
> int status;
> @@ -6738,7 +6737,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
> if (!ret)
> rte_eth_dev_callback_process(dev,
> RTE_ETH_EVENT_INTR_LSC, NULL);
> -
> break;
> default:
> PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", @@
> -12123,40 +12121,6 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
> return ret;
> }
>
> -static void
> -i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) -{
> - struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> - uint32_t rep_cnt = MAX_REPEAT_TIME;
> - struct rte_eth_link link;
> - enum i40e_status_code status;
> - bool can_be_set = true;
> -
> - /*
> - * I40E_MEDIA_TYPE_BASET link up can be ignored
> - * I40E_MEDIA_TYPE_BASET link down that hw->phy.media_type
> - * is I40E_MEDIA_TYPE_UNKNOWN
> - */
> - if (hw->phy.media_type != I40E_MEDIA_TYPE_BASET &&
> - hw->phy.media_type != I40E_MEDIA_TYPE_UNKNOWN) {
> - do {
> - update_link_reg(hw, &link);
> - if (link.link_status)
> - break;
> - rte_delay_ms(CHECK_INTERVAL);
> - } while (--rep_cnt);
> - can_be_set = !!link.link_status;
> - }
> -
> - if (can_be_set) {
> - status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL);
Re: MAC address set requires decision
在 2023/2/23 22:32, Bruce Richardson 写道: On Thu, Feb 23, 2023 at 02:18:59PM +, Ferruh Yigit wrote: On 2/23/2023 4:32 AM, Hemant Agrawal wrote: On 22-Feb-23 11:25 PM, Honnappa Nagarahalli wrote: -Original Message- From: Richardson, Bruce Sent: Thursday, February 16, 2023 9:05 AM To: Ferruh Yigit ; techbo...@dpdk.org Cc: Huisong Li ; Chengwen Feng Subject: RE: MAC address set requires decision Alternative suggestions: 1. Don't allow "set" of mac address to value already in the list. The user must delete the entry manually first before adding it. Similarly, "add" fails if no default mac address is set. This ensures consistency by enforcing strict separation between the default mac address and the extra mac addresses. You can't have extra addresses without a default, and you can't have duplicates. 2. Always enforce overlap between the two lists - once default mac address is set (automatically adding it to the mac addresses list), you can only replace the default mac address by using an already-added one to the list. In this case, the default address is only really an index into the address list, and no deletion ever occurs. All the solutions below seem rather mixed to me, I'd rather see either strict overlap, or strict non-overlap. Both these cases make it that you need more calls to do certain tasks, e.g. with #2 to just replace mac address, you need to add, set, then delete, but we can always add new, clearly named APIs, to do these compound ops. On the plus side, with #2 we could make things doubly clear by changing the parameter type of "set" to be an index, rather than explicit mac, to make it clear what is happening, that you are choosing a default mac from a list of pre-configured options. Regards, /Bruce Both of the above option seems good. The option #1 above is safe, where you are making the mac address set as independent of mac filtering. Also making sure that mac filter are not messed up. However, the application needs to add error handling now to delete and set. In the option #2, I assume, it will provide full backward compatibility i.e. the ethernet library can take care of the logic and application need not to implement anything extra ? If that is the case, it seems to be best. I think #2 is not fully backward compatible, Previously set() replaces MAC in list[0], so multiple set() commands end up with single MAC address. So device will filter only one MAC. With #2, after first set(), application will need to add() first, later set() and del() old MAC, if I understand correctly. prev: set(MAC1) set(MAC2) set(MAC3) becomes: set(MAC1) add(MAC2) set(MAC2) del(MAC1) add(MAC3) set(MAC3) del(MAC2) Hence I think this complicates application that wants to just update default MAC. I agree. I tend to think that #1 is the simplest and most backward-compatible of the options. The only case where we end up with different behaviour is the problematic (and already ambiguous) case where one attempts to set a default mac that is already specified in the "alternative" mac list for the card. It keeps all the simple cases as-is. Hi all, "The user don't allow "set" of mac address to value already in the list. The user must delete the entry manually first before adding it." Is this the final decision? keep it what it was? Do the ethdev framework prohibit the user from setting MAC address that are already in the list? Or, why cannot the ethdev framework explicitly delete the address first before setting it, like patch [1]? Whether there is a problem in this case is completely guaranteed by the user. If the user forget or don't know, there is a another problem as the commit of patch [1] described. [1] https://patches.dpdk.org/project/dpdk/patch/20230202123625.14975-1-lihuis...@huawei.com/ /Huisong .
RE: [PATCH] net/nfp: write link speed to control BAR
> On 2/21/2023 6:29 AM, Chaoyong He wrote:
> > From: James Hershaw
> >
> > Due to changes in the firmware for NFPs, firmware will no longer write
> > the link speed of a port to the control BAR. In line with the
> > behaviour of the kernel NFP driver, this is now handled by the PMD by
> > reading the value provided by the NSP in the nfp_eth_table struct
> > within the pf_dev of the port and subsequently writing this value to the
> control BAR.
> >
>
> Don't you need some kind of FW version check to figure out if
> 'NFP_NET_CFG_STS_NSP_LINK_RATE' needs to be updated by driver or not?
>
> How do you manage driver <-> FW dependency?
>
>
> > Signed-off-by: James Hershaw
> > Reviewed-by: Niklas Söderlund
> > Reviewed-by: Chaoyong He
> > ---
> > drivers/net/nfp/nfp_common.c | 90 ++---
> ---
> > drivers/net/nfp/nfp_ctrl.h | 9
> > 2 files changed, 65 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/nfp/nfp_common.c
> > b/drivers/net/nfp/nfp_common.c index 5922bfea8e..006ea58008 100644
> > --- a/drivers/net/nfp/nfp_common.c
> > +++ b/drivers/net/nfp/nfp_common.c
> > @@ -52,6 +52,53 @@
> > #include
> > #include
> >
> > +static const uint32_t nfp_net_link_speed_nfp2rte[] = {
> > + [NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
> RTE_ETH_SPEED_NUM_NONE,
> > + [NFP_NET_CFG_STS_LINK_RATE_UNKNOWN] =
> RTE_ETH_SPEED_NUM_NONE,
> > + [NFP_NET_CFG_STS_LINK_RATE_1G] =
> RTE_ETH_SPEED_NUM_1G,
> > + [NFP_NET_CFG_STS_LINK_RATE_10G] =
> RTE_ETH_SPEED_NUM_10G,
> > + [NFP_NET_CFG_STS_LINK_RATE_25G] =
> RTE_ETH_SPEED_NUM_25G,
> > + [NFP_NET_CFG_STS_LINK_RATE_40G] =
> RTE_ETH_SPEED_NUM_40G,
> > + [NFP_NET_CFG_STS_LINK_RATE_50G] =
> RTE_ETH_SPEED_NUM_50G,
> > + [NFP_NET_CFG_STS_LINK_RATE_100G]=
> RTE_ETH_SPEED_NUM_100G,
> > +};
> > +
> > +static uint32_t
> > +nfp_net_link_speed_rte2nfp(uint32_t speed) {
> > + uint32_t i;
> > +
> > + for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
> > + if (speed == nfp_net_link_speed_nfp2rte[i])
> > + return i;
> > + }
> > +
> > + return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
> > +}
> > +
> > +static void
> > +nfp_net_notify_port_speed(struct rte_eth_dev *dev) {
> > + struct nfp_net_hw *hw;
> > + struct nfp_eth_table *eth_table;
> > + uint32_t nn_link_status;
> > +
> > + hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > + eth_table = hw->pf_dev->nfp_eth_table;
> > +
> > + nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
> > + nn_link_status = (nn_link_status >>
> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> > + NFP_NET_CFG_STS_LINK_RATE_MASK;
> > +
> > + if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
> > + nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
> > + return;
> > + }
> > +
> > + nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> > + nfp_net_link_speed_rte2nfp(eth_table->ports[hw-
> >idx].speed));
>
> PF driver writes link speed to 'NFP_NET_CFG_STS_NSP_LINK_RATE' register,
> but 'nfp_net_link_update()' still gets it from 'NFP_NET_CFG_STS'
> register (via 'nfp_net_link_speed_nfp2rte[nn_link_status]').
>
> Shouldn't 'nfp_net_link_update()' needs to be updated to read speed from
> 'NFP_NET_CFG_STS_NSP_LINK_RATE' register?
Sorry for the late response, we spend a lot of time to check and discuss.
For older firmware, a full word is allocated (NFP_NET_CFG_STS) to report link
status and port speed to the driver.
However, in the interests of keeping FW files port-speed agnostic in the
future,
the upper 16 bits are no longer written to by FW, so we write the speed to that
address (NFP_NET_CFG_STS_LINK_RATE).
The lower 16 bits (link status) are still handled by firmware.
These changes are completely backwards compatible with older firmware versions,
so no FW version check is required.
[PATCH] log: add timestamp for log
From: zhipeng Lu
add timestamp for log
Signed-off-by: zhipeng Lu
---
lib/eal/common/eal_common_log.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/lib/eal/common/eal_common_log.c b/lib/eal/common/eal_common_log.c
index bd7b188ceb..2b481116b6 100644
--- a/lib/eal/common/eal_common_log.c
+++ b/lib/eal/common/eal_common_log.c
@@ -480,6 +480,27 @@ rte_log_dump(FILE *f)
}
}
+/* get timestamp*/
+void
+rte_log_get_timestamp_prefix(char *buf, int buf_size)
+{
+struct tm *info;
+char date[24];
+struct timespec ts;
+long usec;
+
+clock_gettime(CLOCK_REALTIME, &ts);
+info = localtime(&ts.tv_sec);
+usec = ts.tv_nsec / 1000;
+if (info == NULL) {
+snprintf(buf, buf_size, "[%s.%06ld] ", "unknown date", usec);
+return;
+}
+
+strftime(date, sizeof(date), "%Y-%m-%d %H:%M:%S", info);
+snprintf(buf, buf_size, "[%s.%06ld] ", date, usec);
+}
+
/*
* Generates a log message The message will be sent in the stream
* defined by the previous call to rte_openlog_stream().
@@ -489,6 +510,7 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
{
FILE *f = rte_log_get_stream();
int ret;
+ char timestamp[64];
if (logtype >= rte_logs.dynamic_types_len)
return -1;
@@ -498,7 +520,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
/* save loglevel and logtype in a global per-lcore variable */
RTE_PER_LCORE(log_cur_msg).loglevel = level;
RTE_PER_LCORE(log_cur_msg).logtype = logtype;
-
+ rte_log_get_timestamp_prefix(timestamp, sizeof(timestamp));
+ fprintf(f,"%s ",timestamp);
ret = vfprintf(f, format, ap);
fflush(f);
return ret;
--
2.31.1
[PATCH] log: add timestamp for log
From: zhipeng Lu
add timestamp for log
Signed-off-by: zhipeng Lu
---
lib/eal/common/eal_common_log.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/lib/eal/common/eal_common_log.c b/lib/eal/common/eal_common_log.c
index bd7b188ceb..2b481116b6 100644
--- a/lib/eal/common/eal_common_log.c
+++ b/lib/eal/common/eal_common_log.c
@@ -480,6 +480,27 @@ rte_log_dump(FILE *f)
}
}
+/* get timestamp*/
+void
+rte_log_get_timestamp_prefix(char *buf, int buf_size)
+{
+struct tm *info;
+char date[24];
+struct timespec ts;
+long usec;
+
+clock_gettime(CLOCK_REALTIME, &ts);
+info = localtime(&ts.tv_sec);
+usec = ts.tv_nsec / 1000;
+if (info == NULL) {
+snprintf(buf, buf_size, "[%s.%06ld] ", "unknown date", usec);
+return;
+}
+
+strftime(date, sizeof(date), "%Y-%m-%d %H:%M:%S", info);
+snprintf(buf, buf_size, "[%s.%06ld] ", date, usec);
+}
+
/*
* Generates a log message The message will be sent in the stream
* defined by the previous call to rte_openlog_stream().
@@ -489,6 +510,7 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
{
FILE *f = rte_log_get_stream();
int ret;
+ char timestamp[64];
if (logtype >= rte_logs.dynamic_types_len)
return -1;
@@ -498,7 +520,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
/* save loglevel and logtype in a global per-lcore variable */
RTE_PER_LCORE(log_cur_msg).loglevel = level;
RTE_PER_LCORE(log_cur_msg).logtype = logtype;
-
+ rte_log_get_timestamp_prefix(timestamp, sizeof(timestamp));
+ fprintf(f,"%s ",timestamp);
ret = vfprintf(f, format, ap);
fflush(f);
return ret;
--
2.31.1
