ban lurker questions

[Nils Goroll]

Hi,

I've created https://github.com/varnishcache/varnish-cache/pull/1910 based on
the insights which my questions related to.

FTR regarding phks last response:

>> 1) ban_cleantail
>>
>> why do we acquire the mtx for every ban we look at rather than collecting all
>> the bans to be freed in a loop while holding the mtx?
>
> Probably just an accident of how the code has developed.

implemented.

>> 2) would this assertion be correct?
>>
>> diff --git a/bin/varnishd/cache/cache_ban_lurker.c
>> b/bin/varnishd/cache/cache_ban_lurker.c
>> index 65c552e..fb69f78 100644
>> --- a/bin/varnishd/cache/cache_ban_lurker.c
>> +++ b/bin/varnishd/cache/cache_ban_lurker.c
>> @@ -190,6 +190,7 @@ ban_lurker_test_ban(struct worker *wrk, struct vsl_log *vsl,
>> struct ban *bt,
>>                        VSC_C_main->bans_lurker_obj_killed++;
>>                } else {
>>                        if (oc->ban != bd) {
>> +                               assert(oc->ban == bt);
>>                                Lck_Lock(&ban_mtx);
>>                                oc->ban->refcount--;
>>                                VTAILQ_REMOVE(&oc->ban->objcore, oc, ban_list);
> 
> I am not sure.  Isn't there a window where a HSH_Lookup could race us ?

Yes, absolutely, thank you.

>> 3) ban_lurker_getfirst questions:
>>
>> - for the contention case, shouldn't we continue walking the bt->objcore list
>>  and sleep only if we hit the marker?
> 
> We'd need to come back to the missed oc's later, the lurker cannot skip
> some of the oc's.

Yes, that was clear. I've implemented this.

>> - do IUC correctly that getfirst moves the oc to the tail of the bt->objcore
>>  list, behind the marker, to ensure we don't re-visit ocs which have not got
>>  killed yet, after being handed off to exp?
> 
> Not sure I understand the question...

I was more or less babbling to myself, just re-confirming if my understanding is
correct: ban_lurker_test_ban() inserts oc_marker into the oc list of the ban
being tested. ban_lurker_getfirst() then moves the oc it could lock behind the
marker, which will avoid it to be found again during the same lurker run.

I was not quite sure IUC why the oc is not taken off the ban's list in the first
place, and I presume this is because actual expiry will happen asynchronously in
the exp thread, which should find consistent linkage (ban->oc) in place.

>> 4) ban_lurker_work
>>
>> why do we mark_completed and then clean out the completed bans in the next step
>> rather than removing the completed bans straight away? Why do we need to do the
>> spec fiddling in ban_mark_completed (including a membar (!)) if we're about to
>> ditch the ban anyway?
> 
> again, probably just an accident of how the code developed.

Implemented