Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:

> On Mon, Jun 24, 2024 at 11:36:07AM +0200, Alexander Bluhm wrote:
> > On Fri, Jun 21, 2024 at 06:42:13PM +0200, Alexander Bluhm wrote:
> > > Currently ip_forward() allocates a fake mbuf copy on the stack to
> > > send an ICMP packet after ip_output() has failed.  It seems easier
> > > to just copy the data onto the stack that icmp_error() may use.
> > > Only if the ICMP is acutally sent, create the mbuf.
> > > 
> > > m_dup_pkthdr() uses atomic operation to link the incpb to mbuf.
> > > pf_pkt_addr_changed() is immedately called afterwards to remove the
> > > linkage again.  Also m_tag_delete_chain() is overhead.
> > 
> > Updated diff.  Moves a few lines around to be consistent with my
> > ip6_forward() diff.
> > 
> > anyone?
> 
> The only bit that worries me is that we now need to copy mbuf header
> fields explicitly in this code path. Now icmp_do_error() does the same and
> you provide all the field that code needs. So this is fine.
> Now looking at struct pkthdr I think there is one field that we may want
> to include here as well: ph_loopcnt.
> At least that way we could detect packet loops going through icmp_error()
> in case someone ever manages to build such a case.
> 
> In my opinon both ways of handling this are equally ugly. Your version
> uses a little bit less implicit magic and stack. So OK claudio@

I am happier with this approach than the full-mbuf-on-the-stack.

mbuf lifetime uses a complicated alloc/free API, and I've always worried
about such a mbuf sneaking out and being manipulated in that way, when
it is really a stack object.  I've also worried about such a mbuf reaching
dma.  So it feels way better to keep a real mbuf, but only save the data
for it.  The copy operation involved makes the mbuf data "cache hot" in
the proper way.