Yosry Ahmed <yosryahmed@xxxxxxxxxx> writes: > On Tue, Apr 4, 2023 at 1:12 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote: >> >> Yosry Ahmed <yosryahmed@xxxxxxxxxx> writes: >> >> > On Tue, Mar 28, 2023 at 6:33 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote: >> >> >> >> Yosry Ahmed <yosryahmed@xxxxxxxxxx> writes: >> >> >> >> > On Tue, Mar 28, 2023 at 2:32 PM Chris Li <chrisl@xxxxxxxxxx> wrote: >> >> >> >> >> >> On Tue, Mar 28, 2023 at 02:01:09PM -0700, Yosry Ahmed wrote: >> >> >> > On Tue, Mar 28, 2023 at 1:50 PM Chris Li <chrisl@xxxxxxxxxx> wrote: >> >> >> > > >> >> >> > > On Tue, Mar 28, 2023 at 12:59:31AM -0700, Yosry Ahmed wrote: >> >> >> > > > > > I don't have a problem with this approach, it is not really clean as >> >> >> > > > > > we still treat zswap as a swapfile and have to deal with a lot of >> >> >> > > > > > unnecessary code like swap slots handling and whatnot. >> >> >> > > > > >> >> >> > > > > These are existing code? >> >> >> > > >> >> >> > > Yes. The ghost swap file are existing code used in Google for many years. >> >> >> > > >> >> >> > > > I was referring to the fact that today with zswap being tied to >> >> >> > > > swapfiles we do some necessary work such as searching for swap slots >> >> >> > > > during swapout. The initial swap_desc approach aimed to avoid that. >> >> >> > > > With this minimal ghost swapfile approach we retain this unfavorable >> >> >> > > > behavior. >> >> >> > > >> >> >> > > Can you explain how you can avoid the free swap entry search >> >> >> > > in the swap descriptor world? >> >> >> > >> >> >> > For zswap, in the swap descriptor world, you just need to allocate a >> >> >> > struct zswap_entry and have the swap descriptor point to it. No need >> >> >> > for swap slot management since we are not tied to a swapfile and pages >> >> >> > in zswap do not have a specific position. >> >> >> >> >> >> Your swap descriptor will be using one swp_entry_t, which get from the PTE >> >> >> to lookup, right? That is the swap entry I am talking about. You just >> >> >> substitute zswap swap entry with the swap descriptor swap entry. >> >> >> You still need to allocate from the free swap entry space at least once. >> >> > >> >> > Oh, you mean the swap ID space. We just need to find an unused ID, we >> >> > can simply use an allocating xarray >> >> > (https://docs.kernel.org/core-api/xarray.html#allocating-xarrays). >> >> > This is simpler than keeping track of swap slots in a swapfile. >> >> >> >> If we want to implement the swap entry management inside the zswap >> >> implementation (instead of reusing swap_map[]), then the allocating >> >> xarray can be used too. Some per-entry data (such as swap count, etc.) >> >> can be stored there. I understanding that this isn't perfect (one more >> >> xarray looking up, one more data structure, etc.), but this is a choice >> >> too. >> > >> > My main concern here would be having two separate swap counting >> > implementations -- although it might not be the end of the world. >> >> This isn't a big issue for me. For file systems, there are duplicated >> functionality in different file system implementation, such as free >> block space management. Instead, I hope we can design better swap >> implementation in the future. >> >> > It would be useful to consider all the options. So far, I think we >> > have been discussing 3 alternatives: >> > >> > (a) The initial swap_desc proposal. >> >> My main concern for the initial swap_desc proposal is that the zswap >> code is put in swap core instead of zswap implementation per my >> understanding. So zswap isn't another swap implementation encapsulated >> with a common interface. Please correct me if my understanding isn't >> correct. >> >> If so, the flexibility of the swap system is the cost. For example, >> zswap may be always at the highest priority among all swap devices. We >> can move the cold page from zswap to some swap device. But we cannot >> move the cold page from some swap device to zswap. > > > Not really. In the swap_desc proposal, I intended to have struct > swap_desc contain either a swap device entry (swp_entry_t) or a > frontswap entry (a pointer). zswap implementation would not be in the > swap core, instead, we would have two swap implementations: swap > devices and frontswap/zswap -- each of which implement a common swap > API. We can use one of the free bits to distinguish the type of the > underlying entry (swp_entry_t or pointer to frontswap/zswap entry). > > We can start by only supporting moving pages from frontswap/zswap to > swap devices, but I don't see why the same design would not support > pages moving in the other direction if the need arises. > > The number of free bits in swp_entry_t and pointers is limited (2 bits > on 32-bit systems, 3 bits on 64-bit systems), so there are only a > handful of different swap types we can support with the swap_desc > design, but we only need two to begin with. If in the future we need > more, we can add an indirection layer then or expand swap_desc -- or > we can encode the data within the swap device itself (how it compares > to frontswap/zswap). > > In summary, the swap_desc proposal does NOT involve moving zswap code > to core swap, it involves a generic swap API with two implementations: > swap devices and frontswap/zswap. This eliminate the main concerns for me! Thanks! > The only problems I see with the swap_desc design are: > - Extra overhead for users using swapfiles only. > - A bigger leap from what we have today than other ideas proposed > (e.g. virtual swap device for zswap). Yes. Best Regards, Huang, Ying >> >> >> Maybe compression is always faster than any other swap devices, so we >> will never need the flexibility. Maybe the cost to hide zswap behind a >> common interface is unacceptable. I'm open to these. But please >> provide the evidence, and maybe data. >> >> Best Regards, >> Huang, Ying >> >> > (b) Add an optional indirection layer that can move swap entries >> > between swap devices and add a virtual swap device for zswap in the >> > kernel. >> > (c) Add an optional indirection layer that can move entries between >> > different swap backends. Swap backends would be zswap & swap devices >> > for now. Zswap needs to implement swap entry management, swap >> > counting, etc. >> > >> > Does this accurately summarize what we have discussed so far? >> >
