Ander Juaristi <a@xxxxxxxxxxxx> wrote: > On 13/7/19 18:59, Florian Westphal wrote: > > > >> + if (he == NULL) > >> + return false; > >> + > >> + rhashtable_remove_fast(&priv->ht, &he->node, nft_rhash_params); > >> + return true; > > > > Perhaps add a small comment here that rhashtable_remove_fast retval > > is ignored intentionally? > > > > I.e., don't make this return false in case two cpus race to remove same > > entry. > > Hmm, this made me think. I don't know if this was all too intentional > from me. > > Maybe rather than ignoring it, it would be better to return true only if > rhashtable_remove_fast returned 0, which will only happen if the element > was actually deleted (locking is done internally so two cpus cannot race > in there). Else, if return value is -ENOENT, we should return false. > > And taking this reasoning further, maybe the initial call to > rhashtable_lookup wouldn't be needed either? You need it to obtain he->node, no? Wrt. retval, I might be overthinking it indeed, so making this a "return rhashtable_remove_fast() == 0;" seems fine too, saves the comment :-)
