Hi Paolo,

On 6/24/25 11:04 PM, Lukasz Majewski wrote:

quoted

quoted

On 6/22/25 11:37 AM, Lukasz Majewski wrote:  

quoted

+static void mtip_aging_timer(struct timer_list *t)
+{
+	struct switch_enet_private *fep = timer_container_of(fep,
t,
+
timer_aging); +
+	fep->curr_time = mtip_timeincrement(fep->curr_time);
+
+	mod_timer(&fep->timer_aging,
+		  jiffies +
msecs_to_jiffies(LEARNING_AGING_INTERVAL)); +}    

It's unclear to me why you decided to maintain this function and
timer while you could/should have used a macro around jiffies
instead.  

This is a bit more tricky than just getting value from jiffies.

The current code provides a monotonic, starting from 0 time "base"
for learning and managing entries in internal routing tables for
MTIP.

To be more specific - the fep->curr_time is a value incremented
after each ~10ms.

Simple masking of jiffies would not provide such features.  

I guess you can get the same effect storing computing the difference
from an initial jiffies value and using jiffies_to_msecs(<delta>)/10.

With some coding assuring only 10 bit width of the resulting clock
(based on jiffies) I can have a monotonic clock which will not start
from 0.

quoted

quoted

[...]  

quoted

+static int mtip_sw_learning(void *arg)
+{
+	struct switch_enet_private *fep = arg;
+
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		/* check learning record valid */
+		mtip_atable_dynamicms_learn_migration(fep,
fep->curr_time,
+						      NULL,
NULL);
+		schedule_timeout(HZ / 100);
+	}
+
+	return 0;
+}    

Why are you using a full blown kernel thread here?   

The MTIP IP block requires the thread for learning. It is a HW based
switching accelerator, but the learning feature must be performed by
SW (by writing values to its registers).
  

quoted

Here a timer could
possibly make more sense.  

Unfortunately, not - the code (in
mtip_atable_dynamicms_learn_migration() must be called). This
function has another role - it updates internal routing table with
timestamps (provided by timer mentioned above).  

Why a periodic timer can't call such function?

Yes, the kthread can be replaced with timer with 100ms period.

Just to explain - the mtip_atable_dynamicms_learn_migration(), which
requires monotonic value incremented once per 10ms, is called at two
places:

1. mtip_switch_rx() -> the dynamic table is examined if required (i.e.
new frame arrives). In this place the counter requires 10ms resolution
(can be extracted from jiffies).

2. The mtip_sw_learning() - which now is run from kthread, but it can
be replaced with timer (100ms resolution).

quoted

quoted

Why are checking the table every 10ms, while
the learning intervall is 100ms?   

Yes, this is correct. In 10ms interval the internal routing table is
updated. 100 ms is for learning.
  

quoted

I guess you could/should align the
frequency here with such interval.  

IMHO learning with 10ms interval would bring a lot of overhead.

Just to mention - the MTIP IP block can generate interrupt for
learning event. However, it has been advised (bu NXP support), that
a thread with 100ms interval shall be used to avoid too many
interrupts.  

FTR, my suggestion is to increase the
mtip_atable_dynamicms_learn_migration's call period to 100ms

As mentioned above - it is called in two places. One is in kthread
started at 100ms period, another one is asynchronous when frame arrives.

quoted

quoted

Side note: I think you should move the buffer management to a later
patch: this one is still IMHO too big.  

And this is problematic - the most time I've spent for v13 to
separate the code - i.e. I exclude one function, then there are
warnings that other function is unused (and of course WARNINGS in a
separate patches are a legitimate reason to call for another patch
set revision).  

A trick to break that kind of dependencies chain is to leave a
function implementation empty.

On the same topic, you could have left mtip_rx_napi() implementation
empty up to patch 6 or you could have introduced napi initialization
and cleanup only after such patch.

In a similar way, you could introduce buffer managements in a later
patch and add the relevant calls afterwards.

I get your point.

/P

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de