On Wed 2020-08-26 14:43:22, John Ogness wrote:
> On 2020-08-26, Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx> wrote:
> >>> @@ -1157,6 +1431,14 @@ bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
> >>> goto fail;
> >>> }
> >>>
> >>> + /*
> >>> + * New data is about to be reserved. Once that happens, previous
> >>> + * descriptors are no longer able to be extended. Finalize the
> >>> + * previous descriptor now so that it can be made available to
> >>> + * readers (when committed).
> >>> + */
> >>> + desc_finalize(desc_ring, DESC_ID(id - 1));
> >>> +
> >>> d = to_desc(desc_ring, id);
> >>>
> >>> /*
> >>
> >> Apparently this is not enough to guarantee that past descriptors are
> >> finalized. I am able to reproduce a scenario where the finalization
> >> of a certain descriptor never happens. That leaves the descriptor
> >> permanently in the reserved queried state, which prevents any new
> >> records from being created. I am investigating.
> >
> > Good to know. I also run into problems:
> > - broken dmesg (and broken journalctl -f /dev/kmsg poll) and broken
> > syslog read
> >
> > $ strace dmesg
> >
> > ...
> > openat(AT_FDCWD, "/dev/kmsg", O_RDONLY|O_NONBLOCK) = 3
> > lseek(3, 0, SEEK_DATA) = 0
> > read(3, 0x55dda8c240a8, 8191) = -1 EAGAIN (Resource temporarily unavailable)
> > close(3) = 0
> > syslog(10 /* SYSLOG_ACTION_SIZE_BUFFER */) = 524288
> > mmap(NULL, 528384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f43ea847000
> > syslog(3 /* SYSLOG_ACTION_READ_ALL */, "", 524296) = 0
>
> Yes, this a consequence of the problem. The tail is in the reserved
> queried state, so readers will not advance beyond it.
>
> This series makes a very naive assumption that the previous descriptor
> is either in the reserved or committed queried states. The fact is, it
> can be in any of the 4 queried states. Adding support for finalization
> of all the states then gets quite complex, since any state transition
> (cmpxchg) may have to deal with an unexpected FINAL flag.
>
> The ringbuffer was designed so that descriptors are completely
> self-contained. So adding logic where an action on one descriptor should
> affect another descriptor is far more complex than I initially expected.
>
> Keep in mind the finalization concept satisfies 3 things:
>
> - denote if a record can be extended (i.e. transition back to reserved)
> - denote if a reader may read the record
> - denote if a writer may recycle a record
>
> I have not yet given up on the idea of finalization (particularly
> because it allows mainline LOG_CONT behavior to be preserved locklessy),
In each case, we need to finalize the descriptor also in prb_commit()
when the descriptor is not longer the last reserved one.
It has to be done in two steps to avoid race:
prb_commit()
+ set PRB_COMMIT_MASK
+ check if it is still the last descriptor in the array
+ set PRB_FINAL_MASK when it is not the last descriptor
It should work because prb_reserve() finalizes the previous
descriptor after the new one is reserved. As a result:
+ prb_reserve() should either see PRB_COMMIT_MASK in the previous
descriptor and be able to finalize it.
+ or prb_commit() will see that the head moved and it is not
longer the last reserved one.
I keep my fingers crossed.
Best Regards,
Petr
PS: I am still in the middle of review. And I had a feeling that
this situatuation was not handled.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
