On Wed, May 11, 2022 at 10:29:57AM +0200, Claudio Jeker wrote:
> On Wed, May 11, 2022 at 09:58:09AM +0200, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > >
> > > Can we limit the number of span ports per bridge to a small number so that
> > > the instead of a heap object for the SLIST a simple stack array of
> > > MAX_SPAN_PORTS pointers could be used?
> > >
> > > Who needs more than a handfull of spanports per veb?
> >
> > I just to make sure I follow your idea...
> >
> > >
> > > > --------8<---------------8<---------------8<------------------8<--------
> > > > diff --git a/sys/net/if_veb.c b/sys/net/if_veb.c
> > > > index 2976cc200f1..a02dbac887f 100644
> > </snip>
> > > > + struct veb_span_port *sp;
> > > > + SLIST_HEAD(, veb_span_port) span_list;
> >
> > so the span_list, will be turned to array of references, right?
> >
> > > >
> > > > + SLIST_INIT(&span_list)
> > > > smr_read_enter();
> > > > SMR_TAILQ_FOREACH(p, &sc->sc_spans.l_list, p_entry) {
> > > > ifp0 = p->p_ifp0;
> > > > if (!ISSET(ifp0->if_flags, IFF_RUNNING))
> > > > continue;
> > > >
> > > > - m = m_dup_pkt(m0, max_linkhdr + ETHER_ALIGN, M_NOWAIT);
> > > > - if (m == NULL) {
> > > > - /* XXX count error */
> > > > - continue;
> > > > - }
> > > > + sp = pool_get(&span_port_pool, PR_NOWAIT);
> > > > + if (sp == NULL)
> > > > + continue; /* XXX count error */
> > > >
> > > > - if_enqueue(ifp0, m); /* XXX count error */
> > > > + veb_eb_brport_take(p);
> > > > + sp->sp_port = p;
> > > > + SLIST_INSERT_HEAD(&span_list, sp, sp_entry);
> >
> > here we do instead of SLIST_INSERT_HEAD() something like:
> > span_list[i++] = p;
> >
> > > > }
> > > > smr_read_leave();
> > > > +
> > > > + while (!SLIST_EMPTY(&span_list)) {
> >
> > and here instead of walking list we just walk through array.
>
> Yes, this is my basic idea.
>
> > is that somewhat correct? If so than I need some 'qualified' suggestion
> > for upper limit of span ports. I'm afraid my real life experience
> > with network switches is zero, so I don't feel like a right person.
>
> I would suggest an upper limit of 8. For example it seems Cisco only
> supports 2 span sessions so 8 should be plenty.
Now looking at the original panic again I realized that the problem is
probably not veb_span() but actually veb_broadcast().
Both functions suffer from a similar issue that the smr block covers too
much.
For veb_broadcast the trick with the limit will not work. Using a service
task just for broadcast is also not a good option since it reorders packets.
> > another idea I'm toying with is to add a 'service task' to every switch
> > so the underlying `vport_if_enqueue()` will put packet to queue and
> > let task to dispatch to ifp associated with vport. Once packet
> > will be dispatched the task will drop the reference to port.
>
> Need to look more into this but in general I dislike adding more queues
> and tasks. Such a setup would just synchronize everything back to a single
> thread.
--
:wq Claudio
