Differential D1440
Add return value to MCLGET() to avoid direct testing of M_EXT in device drivers and IPSEC Authored by rwatson on Jan 5 2015, 2:15 PM. Tags None Referenced Files
Details In order to reduce use of M_EXT outside of the mbuf allocator and With this change, very few network device drivers remain aware of Very difficult to test since it modifies countless device drivers for which I do not even have devices; a close review is the best we can do in most cases, so your care in helping to confirm that I've not inverted tests for MCLGET() (or similar bugs) is much appreciated.
Diff Detail
Event TimelineComment Actions Revised diff includes an update to mbuf(9), and is relative to src/ rather than src/sys/. Comment Actions Hm; something about this particular API rubs me up the wrong way, but I can't really put my finger on it. I think I'm more worried about how things are using this particular API than it returning whether the mbuf has external storage on it. So, hm, should you have an API method that tests if an mbuf has external storage or not? Or are you trying very hard to not have that? (The drivers that assume the external storage is of size MCLBYTES is also .. ew.) Comment Actions Hi Adrian: I think the right eventual goal for most of these drivers will be to modify them to request allocation of sufficiently large mbufs (whether due to variable-size support, or via clusters) from initial allocation, rather than attaching clusters to them later. That might not be the ideal model for a driver either, but it accepts the current implementations and refines the KPI around that. However, at this point, my short-term goal is to pull knowledge of M_EXT out of drivers/protocols, and increase their tolerance to storage on mbufs not being what they expect (e.g., getting a cluster when they didn't ask for one, or getting in-mbuf storage when they thought they'd asked for a cluster). An interesting longer-term question is whether to try and eliminate knowledge of MCLSIZE from the device-driver stack: on the one hand, the current KPI makes it very clear what is (and isn't) efficient -- on the other, it's quite an artificial constant, and device drivers perhaps should be requesting mbuf size based on link-layer or protocol properties rather than memory-allocator ones. Anyhow, I think it makes sense to proceed with this patch and then refine these device drivers as the allocator KPI improves .. but we don't plan to stop here. Comment Actions Vast majority of fixed code is "allocate mbuf, then MCLGET()". A year ago I already did a huge sweep that converted many places with conditional MCLGET() to m_get2(). In this case we've got unconditional MCGLGET, and I'd prefer to do a sweep that converts all this stuff to m_getcl(). This change would be even simplier than m_get2-sweep. How many places would remain after, where we MCLGET for already pre-existing mbuf? It might happen there are none. If there is a couple, then we can convert them to m_free() && m_getcl(). In this case we can get rid of MCLGET at all, and lend even less knowledge about clusters and sizes to users of mbuf(9). Comment Actions Agreed entirely with regard to revising memory allocation in device drivers: the current structure is unhelpful. However, in this case I was trying to minimise semantic change. I will have followup changes that do what you suggest (or something similar, anyway), as well as try to scrub many instances of MLEN, MHLEN, and MCLBYTES in device drivers. Thanks, |