On Sat, Feb 24, 2024 at 02:56:15PM -0800, Hao Xiang wrote: > > > > I don't think it's super clean to have three arrays offset, zero and > > > > normal, all sized for the full packet size. It might be possible to just > > > > carry a bitmap of non-zero pages along with pages->offset and operate on > > > > that instead. > > > > > > > > What do you think? > > > > > > > > Peter, any ideas? Should we just leave this for another time? > > > > > > Yeah I think a bitmap should save quite a few fields indeed, it'll however > > > make the latter iteration slightly harder by walking both (offset[], > > > bitmap), process the page only if bitmap is set for the offset. > > > > > > IIUC we perhaps don't even need a bitmap? AFAIU what we only need in > > > Multifdpages_t is one extra field to mark "how many normal pages", aka, > > > normal_num here (zero_num can be calculated from num-normal_num). Then > > > the zero page detection logic should do two things: > > > > > > - Sort offset[] array so that it starts with normal pages, followed up > > > by > > > zero pages > > > > > > - Setup normal_num to be the number of normal pages > > > > > > Then we reduce 2 new arrays (normal[], zero[]) + 2 new fields (normal_num, > > > zero_num) -> 1 new field (normal_num). It'll also be trivial to fill the > > > packet header later because offset[] is exactly that. > > > > > > Side note - I still think it's confusing to read this patch and previous > > > patch separately. Obviously previous patch introduced these new fields > > > without justifying their values yet. IMHO it'll be easier to review if > > > you > > > merge the two patches. > > > > Fabiano, thanks for catching this. I totally missed the backward > > compatibility thing. > > Peter, I will code the sorting and merge this patch with the previous one. > > > It turns out that we still need to add a "zero_pages" field in > MultiFDPacket_t because the existing field "pages_alloc" is not the > total number of pages in "offset". So source can set "zero_pages" from > pages->num - pages->num_normal but "zero_pages" needs to be set in the > packet.
Yes, one more field should be needed in MultiFDPacket_t. Noet that what I said above was about Multifdpages_t, not MultiFDPacket_t (which is the wire protocol instead). To support zero page offloading we should need one more field for each. IMHO MultiFDPacket_t.pages_alloc is redundant and actually not useful.. It's just that it existed in the wire protocol already so maybe we'd still better keep it there.. -- Peter Xu
