intel / intel-ipsec-mb

Intel(R) Multi-Buffer Crypto for IPSec
BSD 3-Clause "New" or "Revised" License
289 stars 87 forks source link

add cipher-direction support #23

Closed kingwelx closed 5 years ago

kingwelx commented 5 years ago

remove chain order check in is_job_invalid refactor submit_new_job & complete_job

kingwelx commented 5 years ago

Sure, will do later. I'll add cases for CBC+CMAC and CTR+CMAC.

tkanteck commented 5 years ago

In the meantime, I ran more extensive functional tests and they all went fine. However, performance tests before and after the patch reveal sudden intercept cycle cost jump for some cipher/mac combinations with the patch. I'll investigate more.

tkanteck commented 5 years ago

Modifying these two functions as follows (see below) seems to results in smallest performance impact (less condition checks I guess). Could you try it please?

__forceinline
JOB_AES_HMAC *submit_new_job(MB_MGR *state, JOB_AES_HMAC *job)
{
        if (job->chain_order == CIPHER_HASH) {
                job = SUBMIT_JOB_AES(state, job);
                if (job != NULL)
                        job = SUBMIT_JOB_HASH(state, job);
        } else {
                job = SUBMIT_JOB_HASH(state, job);
                if (job != NULL)
                        job = SUBMIT_JOB_AES(state, job);
        }

        return job;
}

__forceinline
void complete_job(MB_MGR *state, JOB_AES_HMAC *job)
{
        JOB_AES_HMAC *tmp = NULL;

        while (job->status < STS_COMPLETED) {
                if (job->chain_order == CIPHER_HASH) {
                        tmp = FLUSH_JOB_AES(state, job);
                        if (tmp != NULL)
                                tmp = SUBMIT_JOB_HASH(state, tmp);
                        else
                                tmp = FLUSH_JOB_HASH(state, job);
                } else {
                        tmp = FLUSH_JOB_HASH(state, job);
                        if (tmp != NULL)
                                tmp = SUBMIT_JOB_AES(state, tmp);
                        else
                                tmp = FLUSH_JOB_AES(state, job);
                }
        }
}
kingwelx commented 5 years ago

This is exactly I was thinking in the begining. Unfortuantely it won't work. It can not pass the modified do_test which now will test all chian order and cipher direction combinations.

in sibmit_new_job: job = SUBMIT_JOB_AES(state, job); if (job != NULL) job = SUBMIT_JOB_HASH(state, job);

Here SUBMIT_JOB_AES might return with a HASH_CIHPER job which just completed both HASHing and Ciphering. Then SUBMIT_JOB_HASH simply submits it for double hashing.

Even not like that bad luck, SUBMIT_JOB_HASH might return with a half completed job - a HASH_CIPHER job. This job will never get a chance to be completed.

I think some performance penalty is innevitable, since we are going to add new features. We got to pay price. Maybe it is better to write an iteration instead of while loop? Honestly I don't think there will be any diffrence, but anyway we can try.

kingwelx commented 5 years ago

Well, key message is that we might have all combinations of chain order and cipher direction up and running in the same Job Mgr. Otherwise, we might come up with some sort of simplification.

tkanteck commented 5 years ago

Thanks! let me go back to the drawing board.

tkanteck commented 5 years ago

Your original patch is doing the right thing. I haven't found any potential major optimization of it. I managed to get rid of one if only which saves a few cycles of intercept. I'll run more extensive tests with this small mod and if all good I'll merge.

kingwelx commented 5 years ago

Great. Finaly I gave up making an iteration function for resubmit job, because all functions in mb_mgr_code.h has to be inlined to avoid multiple definition, but iteration doesn't accept inline.

In general, I think while loop is better than function iteration, from performance perspective.

tkanteck commented 5 years ago

Just pushed the commit with some small modes. My current SCM set up has some glitch and a non-author cannot push the commit. You are listed as "co-author" in this commit. Please accept my apologies for it. Many thanks for you contribution.

kingwelx commented 5 years ago

It is ok. Actually you cited my colleague Kane Huang. It is my fault to use his email address for git operation in the beginning. It doesn't matter, we are working in the same team for the same purpose.

Thanks for merging, I don't have to patch DPDK after this.