On Fri, Aug 14, 2009 at 10:58:04AM -0700, Darren Reed wrote: > >no, it won't get freed. > >I just think it's better to not change the expected behavior of > >mpi_fn() because someone passed in a certain flag. I think it's > >more consistent with other mac callbacks to let it consume the > >packet. > > > >If you're concerned about copying cost, you could do dupmsg() instead > >of copymsg() if mpi_no_copy is set. > > No. The entire point of the flag is to avoid any duplication, > be it dupmsg or copymsg. They're simply not required with BPF > and do nothing except making things run slower. When using BPF > to capture packets, there should be no need to allocate any > new data structures in the process of capturing the packet > and the only time packet data is copied is when it matches the > filter being applied to the network traffic. >
I understand that with this flag the mpi_fn() has read-only semantics. my worry is that at some point some code (maybe not bpf) may try to queue the mp instead of completely processing it in the context of mpi_fn(). this is definitely not allowed because mp could be freed from the real datapath. the danger with callbacks is you can't tell how deep they run and you can't guarantee they obey certain retrictions throughout. do you know what's the cost of dupmsg() versus not doing it? if it's not significant, I think it's worthwhile keeping callback semantics consistent in the mac. > Hmmm! > What would be nice is a way to tell mac_header_info() to optionally > iterate through layer 2 headers until the last one is found and > extract the "real" SAP into mhi_bindsap. Looking at > dls_link_header_info(), I can recall looking at it. In its present > form, it isn't useful because it requires a dls_link_t *. But > changing it to use mac_handle_t is quite easy (see attached) and > that would make it possible to use that function rather than writing > another version of it. But changing the first parameter of > dls_link_header_info() implies that the function name and location > should also change...which is getting outside of the scope of what > this project is about. The functional requirement is to provide the > raw ethernet packet. > > So I think what I'll do is: > 1) change the code to mimic dls_link_header_info() > 2) leave the code as is (without the MAC_PROMISC_FLAGS_VLAN_TAG_STRIP > flag) > 3) file a bug that mentions dls_link_header_info() can work with > a mac_handle_t instead of a dls_link_t * and when the mac/dls > cleanup gets into gear, it can take care of both pieces of > code then. See CR#6872007 > sounds fine to me. eric
